Don't swap spaces for commas.

[ajarzyn]

To avoid recipient list friendly name slicing while only one e-mail address is given.

• [ x ] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• [ x ] Ensure that the pull request title represents the desired changelog entry
• [ x ] Please describe what you did
• [ x ] Link to relevant issues in GitHub or Jira
  https://issues.jenkins.io/browse/JENKINS-69681
• [ x ] Link to relevant pull requests, esp. upstream and downstream changes
  N/A?
• [ O ] Ensure you have provided tests - that demonstrates feature works or fixes the issue
  I've edited it in the browser, sorry no environment to run it, and implement tests.

[jeffmaury]

/lgtm

[basil]

Are we worried about backward compatibility implications here?

[slide]

This causes tests to fail. Is the goal here to support the format like First Last <some.email@domain.com> as a single entity?

[ajarzyn]

Are we worried about backward compatibility implications here?

If peaople were using spaces to separate e-mails adresses, then yes that would be a problem.
But beeing frank I do not know any e-mail client that does that. But I may know only handfull of them.

This causes tests to fail. Is the goal here to support the format like First Last <some.email@domain.com> as a single entity?

Yes indeed. Right now to use it this way we need to add comma at the end of the mail to avoid character swapping. It isn't described anywhere and to find it I had to look into the code base.

Online e-mail list splitters by default supports: comma, pipie, colon, new line

ie: https://emailseparator.siitgo.com/

[slide]

We can't break people. Lots of places in Jenkins spaces are used for delimiters in a list. We should just parse this correctly, maybe we can use this: https://docs.oracle.com/javaee/5/api/javax/mail/internet/InternetAddress.html#parse(java.lang.String,%20boolean)

[slide]

The help for the address fields generally say to use commas (https://github.com/jenkinsci/email-ext-plugin/blob/c5914a98487eaaf2ee93fcd0059033a3d5806742/src/main/webapp/help/projectConfig/mailType/recipientList.html for instance) , but that doesn't mean people haven't been using spaces. We would need an upgrade path in a readResolve to look for < in the fields and update the list to use commas and save back to avoid breakage of people I think. I can look into this soon if the original submitted does not have time.

[ajarzyn]

Proper parsing of the e-mails would be way to go, but I'm afraid that there could be a lot of cases that will really complicate implementation.
Consider following:

1. Mixed list of e-mail - which happens more than often in e-mail client:
   user1@email.com User Two <user2@email.com> user3@email.com Four (user4@email.com)
2. Some clients uses parentheses instead of angle brackets () instead of <>
3. Multi part names like:
   User With Multi Name Three (u.three@email.com)
4. Multi language support? (ie. arabic e-mail) - I do not have any experience with this type of e-mails but is it even a thing:
   http://schappo.blogspot.com/2016/03/arabic-email-addresses.html
5. Titles/Names with dot in them (I do not know if that's even a thing)
   U.N. User u.n.user@email.com

I'm not sure if spaces should be allowed as a splitting character at all, the e-mail and the names should be visibly separated, either with adding quotes, using special character in between.

What can be done?
If you do not want to brake backward compatibility, it's possible to leave it as it is, and:

• add different parser for special list type format that would be parsed differently (ie. with putting emails in square brackets and follow different splitting rules [email, email, email]
• add different argument field (ie. -friendlyfrom)
• looking for @ then validating e-mails (https://help.xmatters.com/ondemand/trial/valid_email_format.htm), then checking if the valid e-mail address is boundaried by brackets or angle brackets, and if so get 'previous' e-mail address end and threat everything in between as 'Friendly name'. But again for arabic 'previous' will mean things on the right, when for rest of the language we will have standard left to right lookup.

If we can treat jenkins users as live organism we could:

• make a transition period informing users that this change will be introduced and if they use spaces they should change it.
• it's possible to just threat current implementation as BUGGED and brag about fixing it.

[slide]

How about this?

private static String fixupDelimiters(String input) throws AddressException {
        input = input.trim();
        input = input.replaceAll("\\s+", " ");
        input = input.replaceAll(";", ",");
        if(input.contains(" ") && !input.contains(",") && !input.contains("<")) {
            // fix-up spaces to commas
            input = input.replaceAll(" ", ",");
        }
        return input;
    }

[ajarzyn]

You can't do that check comment with explanation:
#395 (comment)

[roboweaver]

I have the same issue, seems reasonable to make the code a bit smarter here so that an address like Super Admin<super-admin@adobe.com> wouldn't be treated as two addresses.

My workaround is to not use spaces :)

[ajarzyn]

@roboweaver you just reminded me about this PR. I thought it's long closed already.

New (1e501f0) proposition solves:

• using spaces, comas, semicolons as delivmeters
• parsing all strings before e-mail as a part of the personals

There's a side defects: \"name, surname\" the , semicolon will be removed from the name of the user:
hudson.plugins.emailext.recipients.EmailRecipientUtilsTest.testBackwardsNames
org.junit.ComparisonFailure: expected:<Mouse[,] Mickey> but was:<Mouse[] Mickey>

What can be done?

1. We can split the recipient string following \", next we could ignore one in between \" and then glue it back together
2. We can change UT not to include comas in personals

I don't really know why this test is failing:
hudson.plugins.emailext.recipients.EmailRecipientUtilsTest.testValidateFormRecipientList_validationShouldPassAListOfGoodEmailAddresses

Is there an easy way to run tests locally?
@jeffmaury @slide

[slide]

mvn clean package should run all the tests