-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[JENKINS-63539] Expand repo cache lookup for better size estimates #958
Conversation
@MarkEWaite Thanks for doing the great work on PR-947. I have rearranged some of the components as discussed. Apologies for taking too much time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may seem like a strange request, but the output of git log --pretty=oneline --abbrev-commit --decorate
is badly broken when there is not an empty line between the first line of the commit message (the summary, recommend 50 characters or less) and the body of the commit message.
Could you do a git commit --amend
to modify this commit message so that there is a blank line between the first line and the rest of the message? Once you've amended the commit, then you'll need to force the push with git push --force
.
I almost never ask someone to modify a commit message, but this is one of those cases where the output is much harder to read due to the absence of that blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for your good work @rishabhBudhouliya !
Could you review my comments and consider them?
I've added some other cosmetic comments in the form of optional commits to my copy of this branch. Refer to https://github.com/MarkEWaite/git-plugin/commits/JENKINS-63539 to decide if you'd like to include any of them in the pull request. Any that I considered strongly needed are already mentioned in the comments on this pull request. |
Extend the cache lookup process to better use the cached copies. - An in-memory cache that uses a canonical repository URL as its key and stores the repo size - A lookup method which takes the repository URL, converts it into the canonical form and then finds it in the internal cache - A cache assignment method which takes the repository URL, converts it to normalized repository URL format, then stores the size of the repository if the size is larger than the size (optionally) stored in that cache entry already - Look at the internal cache before looking for stored cache in Jenkins.
11421f8
to
33f9efd
Compare
- Since we have included some logs to aid interactive testing, the logging level should be kept to FINE - Order patterns to be matched on the basis of their frequency of usage
@MarkEWaite Many thanks on the further review, I have added your suggestions + one test on the process of normalisation of alternate remote URLs to the standard URL. I will look at the other comments on your fork and try to add them as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks @rishabhBudhouliya . I'll do further interactive testing with the latest build from ci.jenkins.io. I hope to merge the change and release a new git plugin version with the changes this weekend.
JENKINS-63539 - Expand repo cache lookup for better size estimates
Extend the cache lookup process to better use the cached copies.
Note: To avoid potential confusion between the GitToolChooser in-memory cache and the Jenkins cache directory, I have used the term
internal-cache
for the in-memory GitToolChooser cache. We could also name itin-memory
cache if you find that more suitable.Checklist
Types of changes
Further comments
I need to clean/simplify the code, right now it contains duplicated code (most from @MarkEWaite's PR-947).