-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Fix merge of PR with meta/empty commit #19738
Conversation
9140199
to
58dc08c
Compare
|
@Gusted Hi, yeah, because it is not complete. I do not know how to gather the second SHA. And without it, it does not have sense to fix the tests. |
Do you mean of the original branch? As everything in checkConflicts is working on a temporary git repository. |
Mean, when I have pull request with branch A and want to merge it into branch B. Then I'm able to get SHA of branch B. But do not know how to get SHA of branch A. And if I understand, it is not possible or very hard? (unfortunately I'm not too much familiar with golang) Because if I will somehow get the second SHA, then the fix is quite simple. |
You would need to look at |
58dc08c
to
d3e47aa
Compare
Thanks for pointing me to it :) Was indirect but helpful :-D |
Codecov Report
@@ Coverage Diff @@
## main #19738 +/- ##
==========================================
+ Coverage 47.22% 47.30% +0.08%
==========================================
Files 958 957 -1
Lines 133603 133340 -263
==========================================
- Hits 63088 63073 -15
+ Misses 62871 62607 -264
- Partials 7644 7660 +16
Continue to review full report at Codecov.
|
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.
Seems reasonable, no tests seems to be failing. LGTM
Tagging as enhancement as we never had this functionality in the first place. |
@Gusted thanks :) btw I think the tagging just uncovered the issue. The main issue was really about merging two branches. |
Hmm... It might be useful to just jump back to the original PRs that instituted this checking if PRs were empty - I have a feeling that this might be returning to previous behaviour which was undesired if so we might need to keep the empty check here and consider moving the override up to the merge page itself. |
@zeripath And do you know what was undesired behavior and why? |
I'd like to wait for @zeripath 's suggestion. |
Please resolve the conflict. |
* Fixes issue go-gitea#19603 * fill HeadCommitID in PullRequest * compare real commits ID as check for merging
* added explaining comment into patch.go * change log.info into log.debug
d05fae3
to
9e6169c
Compare
OK I will have to take another look at this - but it's too late tonight for me. This PR is breaking empty commit detection as it stands and that's not great. |
@zeripath Not sure what you mean. I think I added both tests. When the commit is different and when it is the same. Or what is your suggestion? |
Have you pushed that up because the code I can see is still the same mergecommitid test? |
You've not applied or progressed the patch I provided that kept correct recognition of empty commits. Most people will not want to be able to merge commits that will make no change to code on the branch. |
@zeripath I meant these tests: TestPullCreate_EmptyChangesWithDifferentCommits, TestPullCreate_EmptyChangesWithSameCommits. Or I should create another PR for it? But you will lost credit for the patch ;-) |
@zeripath Hi, I'm not sure. Should I do an actions? |
I meant the patch in here: #19738 (comment) |
@zeripath So I should create another PR with your changes? If yes, I will do :) |
@zeripath But there is an issue with the patch from you, it seems it allows merge branches, which have identical commit ID. |
As I said it was not a final patch but the beginnings of one... |
@zeripath Awesome. I just upddated the testcase. Also manual test seems ok :) So now waiting for build. Thank you. |
used PR #20290 instead so closing. Thanks to all. |
* Fixes issue go-gitea#19603 (Not able to merge commit in PR when branches content is same, but different commit id) * fill HeadCommitID in PullRequest * compare real commits ID as check for merging * based on @zeripath patch in go-gitea#19738
Hi,
this will fix the issue #19603 regarding different SHA commits, but same content.
The change is to populate HeadCommitID in PullRequest which was previously empty
and compare real SHA commits. It means merge must be always allowed, when there are different SHAs.
Even it the content is same.
Usecase: gitflow
Best regards
Jarek