[JENKINS-38608] Fix to allow null value in 'GIT_URL_n'

[rishabhBudhouliya]

JENKINS-38608

The issue belongs to the Source Code Management section, whereupon the addition of a secondary repository URL and leaving it empty caused something like this java.lang.IllegalArgumentException: Null value not allowed as an environment variable: GIT_URL_2.

Checklist

• I have read the CONTRIBUTING doc
• I have added tests that verify my changes
• Unit tests pass locally with my changes
• I have added documentation as necessary
• No Javadoc warnings were introduced with my changes
• No spotbugs warnings were introduced with my changes
• I have interactively tested my changes
• Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply. Delete the items in the list that do not apply

• Bug fix (non-breaking change which fixes an issue)

[MarkEWaite]

Additional white space changes that I think are needed.

[MarkEWaite]

Thanks for the submission. I had time to do a more detailed code review tonight and wanted to provide you the results of the code review.

I think that the root of the problem is that the repository URL should never be allowed to be null. A UserRemoteConfig with a null repository is not usable for any clone because there is no repository to clone. However, changing the rules for nullability of the URL of a UserRemoteConfig would likely affect compatibility in many places for many users. I am unwilling to take that risk because there are 250 000 installations of the git plugin and compatibility is very important to those installations.

Since I'm unwilling to take the risk of changing the nullabilty of the UserRemoteConfig URL, I think we should improve the error message so that the user understands the mistake they made. Instead of reporting that an environment variable cannot be null, let's tell them that the repository URL cannot be empty.

I also found that the same condition exists if the first repository URL in the job definition is empty. Since the same conditions exist for both the first and any subsequent repository URL's, it seemed safe to insert a check before the assignment of the environment variables.

I've pushed proposed changes to the pull request to a branch in my fork of the git plugin.

Thanks very, very much for proposing this pull request. Would you be willing to review the changes I made and apply the subset you think makes sense into your pull request? I'm perfectly fine if you disagree with something I've done and decide that it needs to be done in a better way.

[MarkEWaite]

Review the comments and code changes in my branch and decide if there is a better way to approach it than I've attempted there.

[fcojfernandez]

I agree with @MarkEWaite. I don't see the current message as understandable or even informative. I have also concerns about side-effects if we just don't set the environment variable, so I my vote here is for giving a better message to users so they can fix the job configuration.

[rishabhBudhouliya]

@MarkEWaite Yes, I definitely agree with the proposed solution, I was not aware of the consequences of allowing a null value in UserRemoteConfig, and clearly I need a better understanding of the plugin.

The only issue I feel here is that from a user perspective, the save button should not allow submission if the Repository URL is left empty. For now, I can't figure out a way to make the field mandatory.

[rishabhBudhouliya]

@MarkEWaite I have reviewed the code and commented on the individual commits.