Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retrieve full name matches over hazy short hash matches #1293

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

RoahNo
Copy link

@RoahNo RoahNo commented Jul 19, 2022

JENKINS-62592 - Return exact matches over fuzzy short hash matches while retrieving revisions.

Not a large change, but this issue addresses the problem arisen by the associated ticket (https://issues.jenkins.io/browse/JENKINS-62592). Commonly, we have seen due to our tagging method/branch name method that we will clash with the hazy short hash revision matching functionality within the AbstractGitSCMSource.retrieve method.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is simply a reminder of what we are going to look for before merging your code. If a checkbox or line does not apply to this pull request, delete it. We prefer all checkboxes to be checked before a pull request is merged

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • 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
  • Documentation in README has been updated as necessary
  • Online help has been added and reviewed for any new or modified fields
  • 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)

Further comments

Change can be summarized and seen in the associated test - instead of returning null when we first clash based on short hash, we store that we have noticed a collision and continue on. This way, if we find a full match, we can return what we see as the expected revision to retrieve.

Let me know if there are any comments or concerns that I am over looking and I look forward to helping in any way I can in this project! Thanks!

@github-actions github-actions bot added dependencies Dependency related change test labels Jul 19, 2022
@RoahNo RoahNo marked this pull request as ready for review July 21, 2022 17:57
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Aug 9, 2022
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Aug 16, 2022
@markhatchell
Copy link

Will this get merged in? I could really use it, the plugin is not select the correct git ref and this looks like it fixes it!

@MarkEWaite
Copy link
Contributor

Will this get merged in? I could really use it, the plugin is not select the correct git ref and this looks like it fixes it!

Not immediately. It has not been reviewed. I did a cursory review and the change looks like a very good start. It includes an automated test (very good). However, it also makes changes that are contrary to the guidelines in the contributing guide (changing white space unnecessarily) and contrary to the guidelines in the SCM API code style guidelines (wild card imports). It will need changes before it is merged.

I have several other reviews that are in my queue ahead of this review.

You could help by installing the incremental build of the plugin in your environment and confirming that it resolves the issue you are seeing and does not have any unexpected side effects.

If you use configuration as code, you can insert this incremental build into your plugins.txt file with the line:

git-client:incrementals;org.jenkins-ci.plugins;git-4.12.2-rc4876.6242d5a_cd5d5

pom.xml Outdated
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>git</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here...twice?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. The comment on the commit indicates that it was intended to be temporary. I've removed it, merged the master branch, and pushed the change

@github-actions github-actions bot removed dependencies Dependency related change test labels Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants