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
1431: The method TestPullRequest#diff sometimes returns wrong information #1360
Conversation
👋 Welcome back gli! A progress list of the required criteria for merging this PR into |
Webrevs
|
// Find the base hash of the source and target branches. | ||
var baseHash = sourceHash; | ||
var targetBranch = new Branch(targetRef()); | ||
while (!"0".repeat(40).equals(baseHash.hex())) { |
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.
So if the common commit is more than 40 commits away, return "no diff"? :-)
This is only used in testing, right? If so, this you could get away with it, but I think a comment describing the limitations would be needed.
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.
I think you are misunderstanding the code. "0".repeat(40)
just generates a string with 40 zeros, basically a null hash.
Looping to find the common ancestor is one way of doing it, but I would recommend checking out git merge-base
. There is a method for it already on GitRepository
.
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.
I see. Yes, I misunderstood the code as looping backwards for a maximum of 40. Don't know how I got to that conclusion. My brain is apparently still partially stuck in vacation mode. I don't think I'm used to methods being called on a string literal.
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.
Looping to find the common ancestor is one way of doing it, but I would recommend checking out
git merge-base
. There is a method for it already onGitRepository
.
Thanks for reminding me. I change the code to use the method mergeBase
just now.
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.
Much better.
@lgxbslgx This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. 🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Thanks for the review. Integrating. /integrate |
Going to push as commit cd765e9. |
Hi all,
The method
TestPullRequest#diff
always passes the head hash of the target branch to the methodRepository#diff
as the base hash. But it may be wrong when the target branch was submitted some new commits after creating the pull request.TestPullRequest#diff
should find the last common hash of the source and target branch as the base hash.This patch fixes the bug and adds the test case. I don't know whether it is good to place the test case at the class
CSRBotTests
, but the methodCSRBotTests#testBackportCsr
indeed contains the situation of this bug.Thanks for taking the time to review.
Best Regards,
-- Guoxiong
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara pull/1360/head:pull/1360
$ git checkout pull/1360
Update a local copy of the PR:
$ git checkout pull/1360
$ git pull https://git.openjdk.org/skara pull/1360/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1360
View PR using the GUI difftool:
$ git pr show -t 1360
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1360.diff