Re: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Matthieu Moy]

Samuel GROOT  writes:

> Can we consider this feature obsolete and remove it?

We're usually quite conservative with backward compatibility. If we
remove the feature, we may want to announce it in the next feature
release and actually remove it in the one after (unless we get valid
objection in the meantime).

I'm all for dropping a feature that no one uses if it turns out to be
the case.

-- 
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Samuel GROOT]

On 05/30/2016 04:20 PM, Matthieu Moy wrote:

Is the "lots of email" format still used?


AFAICT, it was initially supported for backward compatibility, and then
no one removed it, but I wouldn't be surprised if no one actually used
it.

I vaguely remember a message from Ryan Anderson being surprised to see
the old format still supported, but I can't find it in the archives.

In any case:

- git log --grep 'lots of email' => shows only 83b2443
- git log -S'lots of email' => likewise
- git grep 'lots of email' => just one answer in a comment

I'm not sure the feature is even tested.


`grep "non-mbox" t/t9001-send-email.sh` didn't return anything, 
apparently it's not tested.


Can we consider this feature obsolete and remove it?
--
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Matthieu Moy]

Samuel GROOT  writes:

> (mbox) prefix was introduced by Ryan Anderson in 2005 (can't find the
> exact commit though), in opposition with the (non-mbox) format ("lots
> of email") that was used before.

That is actually from the original commit introducing send-email:
83b2443 ([PATCH] Add git-send-email-script - tool to send emails from
git-format-patch-script, 2005-07-31), i.e. ~3 month after Git was born.

At that time, user-friendlyness was not really a priority ;-).

> Is the "lots of email" format still used?

AFAICT, it was initially supported for backward compatibility, and then
no one removed it, but I wouldn't be surprised if no one actually used
it.

I vaguely remember a message from Ryan Anderson being surprised to see
the old format still supported, but I can't find it in the archives.

In any case:

- git log --grep 'lots of email' => shows only 83b2443
- git log -S'lots of email' => likewise
- git grep 'lots of email' => just one answer in a comment

I'm not sure the feature is even tested.

-- 
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Samuel GROOT]

On 05/29/2016 08:05 PM, Matthieu Moy wrote:

Samuel GROOT  writes:


Should we take what Eric suggested (see below) as standard output?


Since the headers are already shown after those lines, it's
redundant to have the entire line.  And we could add
trailers after the headers (with a blank line to delimit):

# existing header output:
From: F 
Cc: A , One 
Subject: foo

# new trailer output
Signed-off-by: SoB 
Acked-by: ack 


I don't think adding the trailers in the output is needed. If the
message says

  Adding foo@example from Signed-off-by trailer

I can guess that it's from "Signed-off-by: foo@example" without having
it explicitly.

Perhaps others think differently, but for me, the shortest output would
be the better (if only to solve the "I never see these lines, they
scrolled out of my terminal" issue).


I agree, the shorter the better.


Ideally, I'd even like to shorten the current output a bit more (the
X-Mailer: header is useless IMHO, and the Date: and Message-id: headers
are probably not useful enough to be shown by default).


Agreed.


(Just thinking aloud, obviously none of this should be a prerequisite to
accept your refactoring patch)


And keep "(mbox) Adding ..." lines as error output, or maybe
displayable by a new option `--verbose`?


I think the "Adding ..." lines make sense by default at least for
beginners (just a few days ago, I received a bunch of test emails by
your team follow by a "Oops, I just noticed you got Cc-ed in my tests"
message ;-), that would probably have been worse without the message).
There could be an advice.* option to deactivate them, though.


An advice.* option seems a good solution to me.


The (mbox) prefix doesn't seem useful to me OTOH, I think it's a
leftover debugging message.


(mbox) prefix was introduced by Ryan Anderson in 2005 (can't find the 
exact commit though), in opposition with the (non-mbox) format ("lots of 
email") that was used before.


Is the "lots of email" format still used?

When adding Cc: from a Signed-off-by: field, (body) prefix is used.
--
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Matthieu Moy]

Samuel GROOT  writes:

> Should we take what Eric suggested (see below) as standard output?
>
>> Since the headers are already shown after those lines, it's
>> redundant to have the entire line.  And we could add
>> trailers after the headers (with a blank line to delimit):
>>
>> # existing header output:
>> From: F 
>> Cc: A , One 
>> Subject: foo
>>
>> # new trailer output
>> Signed-off-by: SoB 
>> Acked-by: ack 

I don't think adding the trailers in the output is needed. If the
message says

  Adding foo@example from Signed-off-by trailer

I can guess that it's from "Signed-off-by: foo@example" without having
it explicitly.

Perhaps others think differently, but for me, the shortest output would
be the better (if only to solve the "I never see these lines, they
scrolled out of my terminal" issue).

Ideally, I'd even like to shorten the current output a bit more (the
X-Mailer: header is useless IMHO, and the Date: and Message-id: headers
are probably not useful enough to be shown by default).

(Just thinking aloud, obviously none of this should be a prerequisite to
accept your refactoring patch)

> And keep "(mbox) Adding ..." lines as error output, or maybe
> displayable by a new option `--verbose`?

I think the "Adding ..." lines make sense by default at least for
beginners (just a few days ago, I received a bunch of test emails by
your team follow by a "Oops, I just noticed you got Cc-ed in my tests"
message ;-), that would probably have been worse without the message).
There could be an advice.* option to deactivate them, though.

The (mbox) prefix doesn't seem useful to me OTOH, I think it's a
leftover debugging message.

-- 
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Samuel GROOT]

On 05/28/2016 05:04 PM, Matthieu Moy wrote:

Eric Wong  writes:


Samuel GROOT  wrote:


   (mbox) Adding cc: A  from line 'Cc: A, 
One'
   (mbox) Adding cc: One  from line 'Cc: A, 
One'

Though `git send-email` now outputs something like:

   (mbox) Adding cc: A  from line 'Cc: 
A'
   (mbox) Adding cc: One  from line 'Cc: 
One'

I actually like neither, and would prefer something shorter:

(mbox) Adding cc: A  from Cc: header
(mbox) Adding cc: One  from Cc: header
(mbox) Adding cc: SoB  from Signed-off-by: trailer

That way, there's no redundant addresses in each line and less
likely to wrap.


I agree with this. Actually, I'd even say that the current output of
"git send-email" looks sloppy, and internal refactoring may be a good
opportunity to get it cleaner.


I agree.

Should we take what Eric suggested (see below) as standard output?


Since the headers are already shown after those lines, it's
redundant to have the entire line.  And we could add
trailers after the headers (with a blank line to delimit):

# existing header output:
From: F 
Cc: A , One 
Subject: foo

# new trailer output
Signed-off-by: SoB 
Acked-by: ack 


And keep "(mbox) Adding ..." lines as error output, or maybe displayable 
by a new option `--verbose`?

--
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Matthieu Moy]

Eric Wong  writes:

> Samuel GROOT  wrote:
>
>>(mbox) Adding cc: A  from line 'Cc: 
>> A, One'
>>(mbox) Adding cc: One  from line 'Cc: 
>> A, One'
>> 
>> Though `git send-email` now outputs something like:
>> 
>>(mbox) Adding cc: A  from line 'Cc: 
>> A'
>>(mbox) Adding cc: One  from line 'Cc: 
>> One'
> I actually like neither, and would prefer something shorter:
>
> (mbox) Adding cc: A  from Cc: header
> (mbox) Adding cc: One  from Cc: header
> (mbox) Adding cc: SoB  from Signed-off-by: trailer
>
> That way, there's no redundant addresses in each line and less
> likely to wrap.

I agree with this. Actually, I'd even say that the current output of
"git send-email" looks sloppy, and internal refactoring may be a good
opportunity to get it cleaner.

-- 
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Eric Wong]

Samuel GROOT  wrote:
> While working on the new option `quote-email`, we needed to parse an
> email file. Since the work is already done, but the parsing and data
> processing are in the same loop, we wanted to refactor the parser, to
> make the code more maintainable.

Thank you for doing this work :)

> This is still WIP, and one of the main issue (and we need your
> advice on that), is that around 30 tests will fail, and most of them
> are false-negatives: to pass, they rely on the format of what is
> displayed by `git send-email`, not only data.
> 
> 
> For example, several tests will fail because they do a strict compare
> between `git send-email`'s output and:
> 
>(mbox) Adding cc: A  from line 'Cc: 
> A, One'
>(mbox) Adding cc: One  from line 'Cc: 
> A, One'
> 
> Though `git send-email` now outputs something like:
> 
>(mbox) Adding cc: A  from line 'Cc: 
> A'
>(mbox) Adding cc: One  from line 'Cc: 
> One'
I actually like neither, and would prefer something shorter:

(mbox) Adding cc: A  from Cc: header
(mbox) Adding cc: One  from Cc: header
(mbox) Adding cc: SoB  from Signed-off-by: trailer

That way, there's no redundant addresses in each line and less
likely to wrap.

But I actually never noticed these lines in the past since they
scrolled off my terminal :x

Since the headers are already shown after those lines, it's
redundant to have the entire line.  And we could add
trailers after the headers (with a blank line to delimit):

# existing header output:
From: F 
Cc: A , One 
Subject: foo

# new trailer output
Signed-off-by: SoB 
Acked-by: ack 

> We can think of several solutions:
> 
>1. Modify the parser, to give the script the knowledge of the exact
>   line the data came from.
> 
>2. Hack the tests: modify the script using the parser to artificially
>   recreate the supposedly parsed line.
>   (e.g. with a list.join(', ')-like function)
> 
>3. Modify the tests to replace exact cmp by greps.
> 
> 
> IMHO, we should consider option 3, since the tests should rely on data
> rather than exact outputs. It also makes the tests more maintainable,
> in the sense that they will be more resilient to future output
> modifications.

Agreed on 3.

I am not sure if anybody outside of tests parses the stdout of
send-email.  It's certainly a porcelain and I don't think
stdout needs to be stable, and maybe the output in
question should go to stderr since it could be considered
debug output.

But I could be wrong...
--
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

[WIP-PATCH 0/2] send-email: refactor the email parser loop

[Samuel GROOT]

While working on the new option `quote-email`, we needed to parse an
email file. Since the work is already done, but the parsing and data
processing are in the same loop, we wanted to refactor the parser, to
make the code more maintainable.

This is still WIP, and one of the main issue (and we need your
advice on that), is that around 30 tests will fail, and most of them
are false-negatives: to pass, they rely on the format of what is
displayed by `git send-email`, not only data.


For example, several tests will fail because they do a strict compare
between `git send-email`'s output and:

   (mbox) Adding cc: A  from line 'Cc: 
A, One'
   (mbox) Adding cc: One  from line 'Cc: 
A, One'

Though `git send-email` now outputs something like:

   (mbox) Adding cc: A  from line 'Cc: 
A'
   (mbox) Adding cc: One  from line 'Cc: 
One'

The new behavior is explained because parsing and processing are not
done in the same function, and processing cannot know the exact line
the data came from.

We can think of several solutions:

   1. Modify the parser, to give the script the knowledge of the exact
  line the data came from.

   2. Hack the tests: modify the script using the parser to artificially
  recreate the supposedly parsed line.
  (e.g. with a list.join(', ')-like function)

   3. Modify the tests to replace exact cmp by greps.


IMHO, we should consider option 3, since the tests should rely on data
rather than exact outputs. It also makes the tests more maintainable,
in the sense that they will be more resilient to future output
modifications.

What do you think?


   [WIP-PATCH 1/2] send-email: create email parser subroutine
   [WIP-PATCH 2/2] send-email: use refactored subroutine to parse patches

git-send-email.perl | 284 

  1 file changed, 164 insertions(+), 120 deletions(-)

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