From: Felipe Contreras Date: Mon, 06 Feb 2012 01:27:50 GMT Subject: Re: [PATCH] send-email: add extra safetly in address sanitazion Message-ID: In-Reply-To: <877h01rlsr.fsf@thomas.inf.ethz.ch> On Sun, Feb 5, 2012 at 11:51 PM, Thomas Rast wrote: > Felipe Contreras writes: > >> 2012/2/5 Thomas Rast : >>> Felipe Contreras writes: >>>> >>>>   'Foo Bar ' -> 'Foo Bar ' >>>>   '"Foo Bar" ' -> '"Foo Bar" ' >>>>   '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' > > 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 ' >>>>   '"Foo Bar" >' -> '"Foo Bar" ' >>>>   '>' -> '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. --- > 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 " err blop", or any number of other malformed strings. > 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. > 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 ", 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