Re: [PATCH] send-email: add extra safetly in address sanitazion

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 6, 2012, 01:27 UTC
Message-ID: <CAMP44s30VmJasMLJxs-JFwksvPEPpG1LB3Gr_pA2+hpE1AnwXg@mail.gmail.com>
In-Reply-To: <877h01rlsr.fsf@thomas.inf.ethz.ch>

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

Show 36 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 2012/2/5 Thomas Rast <trast@inf.ethz.ch>:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>   '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'
>>>
>>> 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?
>>
>> There is: '<foo@bar.com>' -> 'foo@bar.com'
>
> 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.
>
>>>>   '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'
>>>
>>> All of these are the same underlying issue.  Does your patch fix any
>>> other malformed addresses, or just this particular type?
>>
>> See above.
>
> 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.

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

Show 5 quoted lines

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

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

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

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

Show 11 quoted lines

> I see that you wrote in another thread:
>
>> I have to write a peer-reviewed essay with an introduction for the
>> people that are not familiar with the code in each of the patches
>
> 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.

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.

Show 5 quoted lines

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

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