Fix merge of PR with meta/empty commit by jedi7 · Pull Request #19738 · go-gitea/gitea

jedi7 · 2022-05-18T06:39:12Z

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

merge from develop -> release (no changes)
merge from release -> master (no changes)
tag master
merge from master -> develop (no file changes ! just meta commit with merging note)

Best regards
Jarek

Gusted · 2022-05-21T14:30:01Z

TestPullCreate_EmptyChangesWithCommits is currently failing.

jedi7 · 2022-05-21T15:12:18Z

@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.

Gusted · 2022-05-21T21:46:47Z

I'm able to get sha commit for the target branch, but I have no idea how to get SHA for the source branch.
Can somebody point me to right direction please?

Do you mean of the original branch? As everything in checkConflicts is working on a temporary git repository.

jedi7 · 2022-05-22T04:39:55Z

I'm able to get sha commit for the target branch, but I have no idea how to get SHA for the source branch.
Can somebody point me to right direction please?

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.

Gusted · 2022-05-22T08:57:20Z

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 head references, so in this case you might want to use pr.HeadCommitID.

jedi7 · 2022-05-23T08:14:54Z

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 head references, so in this case you might want to use pr.HeadCommitID.

Thanks for pointing me to it :) Was indirect but helpful :-D

codecov-commenter · 2022-05-26T04:14:28Z

Codecov Report

Merging #19738 (af39d85) into main (e82db15) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            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

Impacted Files	Coverage Δ
routers/web/repo/pull.go	`31.54% <100.00%> (+0.30%)`	⬆️
services/pull/patch.go	`56.61% <100.00%> (+1.03%)`	⬆️
models/db/context.go	`70.10% <0.00%> (-14.90%)`	⬇️
models/repo/user_repo.go	`0.00% <0.00%> (-8.89%)`	⬇️
models/organization/org_user.go	`92.53% <0.00%> (-4.61%)`	⬇️
models/repo/repo_indexer.go	`47.69% <0.00%> (-3.74%)`	⬇️
modules/context/package.go	`56.75% <0.00%> (-2.95%)`	⬇️
models/auth/oauth2.go	`60.12% <0.00%> (-2.84%)`	⬇️
models/asymkey/ssh_key_authorized_principals.go	`6.66% <0.00%> (-2.20%)`	⬇️
models/project/project.go	`38.25% <0.00%> (-2.07%)`	⬇️
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e82db15...af39d85. Read the comment docs.

Gusted

Seems reasonable, no tests seems to be failing. LGTM

Gusted · 2022-05-27T11:59:59Z

Tagging as enhancement as we never had this functionality in the first place.

jedi7 · 2022-05-27T12:09:12Z

@Gusted thanks :) btw I think the tagging just uncovered the issue. The main issue was really about merging two branches.

zeripath · 2022-06-05T08:20:07Z

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.

jedi7 · 2022-06-05T14:39:43Z

@zeripath And do you know what was undesired behavior and why?

wxiaoguang · 2022-06-12T13:34:08Z

I'd like to wait for @zeripath 's suggestion.

lunny · 2022-06-16T05:42:56Z

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

zeripath · 2022-06-16T21:57:11Z

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.

jedi7 · 2022-06-21T06:01:16Z

@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?

zeripath · 2022-06-21T06:03:40Z

Have you pushed that up because the code I can see is still the same mergecommitid test?

zeripath · 2022-06-21T06:06:56Z

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.

jedi7 · 2022-06-21T06:16:54Z

@zeripath I meant these tests: TestPullCreate_EmptyChangesWithDifferentCommits, TestPullCreate_EmptyChangesWithSameCommits.
Anyway If you want to merge your more sophisticated patch, I will be happy too :)
I did not know I should add your changes to mine (it will probably create a mess).

Or I should create another PR for it? But you will lost credit for the patch ;-)

jedi7 · 2022-07-07T06:43:29Z

@zeripath Hi, I'm not sure. Should I do an actions?

zeripath · 2022-07-07T20:12:06Z

I meant the patch in here: #19738 (comment)

jedi7 · 2022-07-08T06:20:38Z

@zeripath So I should create another PR with your changes? If yes, I will do :)

jedi7 · 2022-07-08T07:14:32Z

@zeripath I created PR based on your changes: #20290

jedi7 · 2022-07-10T19:09:47Z

@zeripath But there is an issue with the patch from you, it seems it allows merge branches, which have identical commit ID.
Also I have now less time to keep alive two PR :( When I do not know if it will be even merged. It happened to me before where I created two PR dependent on different person opinions , and none was merged in final. So wasted time. :(

zeripath · 2022-07-10T19:11:02Z

As I said it was not a final patch but the beginnings of one...

zeripath · 2022-07-11T20:42:23Z

@zeripath But there is an issue with the patch from you, it seems it allows merge branches, which have identical commit ID.

OK I've fixed the check in #20290 but the PRs need to be retested to update to the new status.

jedi7 · 2022-07-12T15:28:47Z

@zeripath Awesome. I just upddated the testcase. Also manual test seems ok :) So now waiting for build. Thank you.

@zeripath

* Fixes issue #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 #19738

jedi7 · 2022-07-13T08:34:48Z

used PR #20290 instead so closing. Thanks to all.

@zeripath

* 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

jedi7 force-pushed the bugfix/metacommit_merging branch 3 times, most recently from 9140199 to 58dc08c Compare May 18, 2022 09:32

jedi7 changed the title ~~WIP: Fix issue #19603~~ WIP: Fix merge of PR with meta/empty commit May 20, 2022

jedi7 changed the title ~~WIP: Fix merge of PR with meta/empty commit~~ WIP: Fix merge of PR with meta/empty commit (advice needed) May 21, 2022

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 21, 2022

Gusted added this to the 1.17.0 milestone May 22, 2022

Gusted added the type/bug label May 22, 2022

jedi7 force-pushed the bugfix/metacommit_merging branch from 58dc08c to d3e47aa Compare May 23, 2022 06:02

jedi7 changed the title ~~WIP: Fix merge of PR with meta/empty commit (advice needed)~~ Fix merge of PR with meta/empty commit May 23, 2022

jedi7 marked this pull request as ready for review May 23, 2022 07:49

Gusted reviewed May 25, 2022

View reviewed changes

Comment thread integrations/pull_status_test.go Outdated

Comment thread services/pull/patch.go Outdated

zeripath reviewed May 25, 2022

View reviewed changes

Comment thread services/pull/patch.go Outdated

Gusted approved these changes May 27, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 27, 2022

Gusted added type/enhancement An improvement of existing functionality and removed type/bug labels May 27, 2022

lunny requested a review from zeripath June 4, 2022 13:10

jedi7 added 3 commits June 16, 2022 09:02

Fix checks in PR for empty commits go-gitea#19603

f95fb7d

* Fixes issue go-gitea#19603 * fill HeadCommitID in PullRequest * compare real commits ID as check for merging

Fix integration test TestPullCreate_EmptyChanges..

4aab46f

Add test for same commits in PR

9e6169c

* added explaining comment into patch.go * change log.info into log.debug

jedi7 force-pushed the bugfix/metacommit_merging branch from d05fae3 to 9e6169c Compare June 16, 2022 07:02

lunny modified the milestones: 1.17.0, 1.18.0 Jun 17, 2022

lunny added the backport/v1.17 label Jun 17, 2022

jedi7 mentioned this pull request Jul 8, 2022

Fix checks in PR for empty commits #19603 #20290

Merged

jedi7 closed this Jul 13, 2022

6543 removed the backport/v1.17 label Jul 29, 2022

6543 removed this from the 1.18.0 milestone Jul 29, 2022

go-gitea locked and limited conversation to collaborators May 3, 2023

Uh oh!

Conversation

jedi7 commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gusted commented May 21, 2022

Uh oh!

jedi7 commented May 21, 2022

Uh oh!

Gusted commented May 21, 2022

Uh oh!

jedi7 commented May 22, 2022

Uh oh!

Gusted commented May 22, 2022

Uh oh!

jedi7 commented May 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 26, 2022

Codecov Report

Uh oh!

Gusted left a comment

Choose a reason for hiding this comment

Uh oh!

Gusted commented May 27, 2022

Uh oh!

jedi7 commented May 27, 2022

Uh oh!

zeripath commented Jun 5, 2022

Uh oh!

jedi7 commented Jun 5, 2022

Uh oh!

wxiaoguang commented Jun 12, 2022

Uh oh!

lunny commented Jun 16, 2022

Uh oh!

zeripath commented Jun 16, 2022

Uh oh!

jedi7 commented Jun 21, 2022

Uh oh!

zeripath commented Jun 21, 2022

Uh oh!

zeripath commented Jun 21, 2022

Uh oh!

jedi7 commented Jun 21, 2022

Uh oh!

jedi7 commented Jul 7, 2022

Uh oh!

zeripath commented Jul 7, 2022

Uh oh!

jedi7 commented Jul 8, 2022

Uh oh!

jedi7 commented Jul 8, 2022

Uh oh!

jedi7 commented Jul 10, 2022

Uh oh!

zeripath commented Jul 10, 2022

Uh oh!

zeripath commented Jul 11, 2022

Uh oh!

jedi7 commented Jul 12, 2022

Uh oh!

jedi7 commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jedi7 commented May 18, 2022 •

edited

Loading