On Sun, Feb 5, 2012 at 11:51 PM, Thomas Rast <trast@xxxxxxxxxxx> wrote: > Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: > >> 2012/2/5 Thomas Rast <trast@xxxxxxxxxxx>: >>> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: >>>> >>>> 'Foo Bar <foo@xxxxxxx>' -> 'Foo Bar <foo@xxxxxxx>' >>>> '"Foo Bar" <foo@xxxxxxx>' -> '"Foo Bar" <foo@xxxxxxx>' >>>> 'foo@xxxxxxx' -> 'foo@xxxxxxx' >>>> '<foo@xxxxxxx>' -> 'foo@xxxxxxx' >>>> '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@xxxxxxx>' -> 'foo@xxxxxxx' > > 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@xxxxxxx>>' -> 'Foo Bar <foo@xxxxxxx>' >>>> '"Foo Bar" <foo@xxxxxxx>>' -> '"Foo Bar" <foo@xxxxxxx>' >>>> '<foo@xxxxxxx>>' -> 'foo@xxxxxxx' >>> >>> 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 "<foo@xxxxxxx> 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 <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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html