patchsend-email: add extra safetly in address sanitazion

Currently bad addresses like 'Foo Bar <foo@bar.com>>' will just be sent verbatim -- that's not good; we should either error out, or sanitize them.

The following patch adds extra sanitazion so the following transformations are performed:

  'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
  '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
  'foo@bar.com' -> 'foo@bar.com'
  '<foo@bar.com>' -> 'foo@bar.com'
  'Foo Bar' -> 'Foo Bar'
  'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
  '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
  '<foo@bar.com>>' -> 'foo@bar.com'

Basically, we try to check that the address is in the form of "Name <email>", and if not, assume it's "email". According to commit 155197e[1], the "prhase" should not be empty, so if it is, remove the <>. Extra characters after the first ">" are ignored.

[1] send-email: rfc822 forbids using <address@domain> without a non-empty "phrase"

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-send-email.perl |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

Felipe Contreras <felipe.contreras@gmail.com> writes:

Am I the only one who stared at this for ten seconds, only to then realize that there is no sanitizing whatsoever going on here?

All of these are the same underlying issue. Does your patch fix any other malformed addresses, or just this particular type?

                   ^^^^^^
"phrase"

Tests?

2012/2/5 Thomas Rast <trast@inf.ethz.ch>:

There is: '<foo@bar.com>' -> 'foo@bar.com'

See above.

Felipe Contreras <felipe.contreras@gmail.com> writes:

Indeed.

I still feel cheated as a reader though, you showed me four examples of no change but let me figure that on my own.

Ok, I see I am falling into the same communication trap as Jonathan, so let's be more explicit.

Your commit message first tells me you are going to sanitize something, but starts out with examples of leaving the string unchanged. Then it continues with only the '>>' examples.

Today, and being someone who on average reads about half the mail that comes through here, I know that this relates to the blame -e '>>' bug. So today, I am wondering from the commit message why you narrowly focus on that bug. But you don't! It's just that the commit message insinuates it.

In a year, your reader (and bear in mind that this may very well be yourself, at least if your memory is as good as mine) will wonder what was so damn special about that '>>' string that it needs a specific fix to send-email.

I see that you wrote in another thread:

I'm not sure you meant it that literally, but the whole *point* is that the message is for people who are not familiar with the code. After all, if I knew that your code did the right thing in the right way, I would not be bothering with reading the message. Today, I would just send an Acked-by instead. In a year, I'd scroll down for another potential culprit for the bug I'm hunting.

What's especially striking me about your proposed messages of late: they leave me with more open questions than I started with. I tried to show this above. I'm not sure whether other contributors are better at answering questions, or just better at not touching any topics that might raise them.

On Sun, Feb 5, 2012 at 11:51 PM, Thomas Rast <trast@inf.ethz.ch> wrote:

Which is why I added a paragraph to explain them. What is unclear about?

--- According to commit 155197e[1], the "prhase" should not be empty, so if it is, remove the <>. Extra characters after the first ">" are ignored. ---

The summary explains the purpose of the patch "add extra safety in address sanitation" (should fix those typos though).

It doesn't matter, could be "<foo@bar.com> err blop", or any number of other malformed strings.

You are assuming too much. In this case, the code is clear and doesn't need explaining. I am talking about other cases which in my mind are akin to explaining what is $recipient, $recipient_name, and what does sanitize_address does, and why the if case for is_rfc2047_quoted is modified. IMO that's overkill.

If you have some suggestion about how to improve the commit message, I would be glad to listen to them, as in this case, I do believe the changes merit some clear explanation. Not all patches do, though.

Again, what is not clear about:

--- Basically, we try to check that the address is in the form of "Name <email>", and if not, assume it's "email". According to commit 155197e[1], the "prhase" should not be empty, so if it is, remove the <>. Extra characters after the first ">" are ignored. ---

To me that explains what the patch is trying to do: "add extra safety in address sanitation".

Anyway, it seems people don't care if 'git send-email' attempts to send random garbage regardless, so I'm not going to pursue this patch.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

I actually think people _do_ care, and that is the _only_ reason you are getting review comments.

Thomas Rast <trast@inf.ethz.ch> writes:

Just this particular type, as long as the code handles it correctly, would be better than nothing.

On the recieving end in mailinfo, I think we also support

	gitster@pobox.com (Junio C Hamano)

although it does not seem to be used by many people these days (the only one I can remember seeing on this list was merlyn), so we may want to be a bit more consistent between sending and receiving end, though.