allow mark PR as manually merged by push option #21951

a1012112796 · 2022-11-27T06:11:28Z

when push with push option merge-pulls=pull_index1[,pull_index2,...] will mark chosed pull requests as manually_merge_pull_confirmed, then in manuallyMerged check, will skip config.AutodetectManualMerge and continue check whether these prs has been manually merged, if yes, the result will be same as Autodetect manual merge, else will reset the manually_merge_pull_confirmed flag. I think this enhancement can make "Mark PR as manually merged" and "Autodetect manual merge" have same behavior. Thanks.

fix #19528

todo:

maybe remove manually-merged merge option after this pr.

Signed-off-by: a1012112796 1012112796@qq.com

example:

follow go-gitea#12543 when push with push option `merge-pulls=pull_index1[,pull_index2,...]` will mark chosed pull requests as `manually_merge_pull_confirmed`, then in `manuallyMerged` check, will skip config.AutodetectManualMerge and continue check whether these prs has been manually merged, if yes, the result will be same as `Autodetect manual merge`, else will reset the `manually_merge_pull_confirmed` flag. I think this enhancement can make "Mark PR as manually merged" and "Autodetect manual merge" have same behavior. Thanks. fix go-gitea#19528 Signed-off-by: a1012112796 <1012112796@qq.com>

models/migrations/migrations.go

Co-authored-by: Yarden Shoham <hrsi88@gmail.com>

models/issues/pull.go

routers/private/hook_post_receive.go

Co-authored-by: Yarden Shoham <hrsi88@gmail.com>

Signed-off-by: a1012112796 <1012112796@qq.com>

zeripath · 2022-12-16T11:15:24Z

models/issues/pull.go

+// ManuallyMergePullConfirm confirm manually merge pulls by repo id and indexes
+func ManuallyMergePullConfirmByIndexes(ctx context.Context, repoID int64, indexes []int64) error {
+	_, err := db.GetEngine(ctx).Where(builder.And(
+		builder.Eq{"base_repo_id": repoID},
+		builder.Eq{"has_merged": false},
+		builder.In("`index`", indexes),
+	)).Cols("manually_merge_pull_confirmed").Update(&PullRequest{
+		ManuallyMergePullConfirmed: true,
+	})
+
+	return err
+}


This doesn't appear to do any permissions chcek?

Hmm, In my view, This is in the post hook, that meaning the permission check of pushing to the base branch has passed in pre-recevice hoook. So not need check it again.if the pull request isn't be merged,nothing will happend after pull check.

zeripath · 2022-12-16T14:02:24Z

routers/private/hook_post_receive.go

+		mergePullIndexes := opts.GitPushOptions.Int64Array(private.GitPushOptionMergePulls)
+		if len(mergePullIndexes) > 0 {
+			if err := issues_model.ManuallyMergePullConfirmByIndexes(ctx, repo.ID, mergePullIndexes); err != nil {
+				log.Error("Failed to manually merge pull confirm: %s/%s Error: %v", ownerName, repoName, err)
+				ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
+					Err: fmt.Sprintf("Failed to manually merge pull confirm: %s/%s Error: %v", ownerName, repoName, err),
+				})
+				return
+			}


this doesn't appear to do any permissions checks?

Why not just do the manualMerge checks here instead of hacking in this new ManuallyMergePullConfirm flag?

I think if do it here, It will be duplicate with the 'pull check' logic. add a single flag to ignore AutodetectManualMerge config is the more simple way.

added an extern limit,only mark pull request as merge confirmed if the base branch is updated in this push event, How about this state?
0b3ef59

But by not doing the check when processing the receive-hook you risk the flag being reset to false before the pull is actually processed. Remember these two things are occurring in totally different goroutines and may be occurring semi-concurrently.

There's no guarantee that the next time manuallyMerged() is run relates to this particular push.

Hmm, But checking pull reuest will cost much time, should not do it in hook. it's really a hard problem ...
maybe use a counter instead of a boolen flag?

@zeripath how about use version for sync? Thanks. b8ffb32

Signed-off-by: a1012112796 <1012112796@qq.com>

codecov-commenter · 2022-12-20T13:23:12Z

Codecov Report

❗ No coverage uploaded for pull request base (main@4d20a4a). Click here to learn what that means.
The diff coverage is 13.63%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main   #21951   +/-   ##
=======================================
  Coverage        ?   47.26%           
=======================================
  Files           ?     1111           
  Lines           ?   149456           
  Branches        ?        0           
=======================================
  Hits            ?    70636           
  Misses          ?    70431           
  Partials        ?     8389

Impacted Files	Coverage Δ
models/issues/pull.go	`49.35% <0.00%> (ø)`
routers/private/hook_post_receive.go	`58.70% <5.26%> (ø)`
modules/private/hook.go	`3.20% <18.18%> (ø)`
services/pull/check.go	`27.54% <37.50%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lunny · 2023-02-03T06:42:56Z

Please resolve the conflicts

yardenshoham · 2023-02-04T14:14:09Z

modules/private/hook.go

 	GitPushOptionRepoPrivate  = "repo.private"
 	GitPushOptionRepoTemplate = "repo.template"
+	GitPushOptionMergePulls   = "merge-pulls"


Would it make more sense to name it pulls.merge to keep using the same naming convention?

Signed-off-by: a1012112796 <1012112796@qq.com>

docs/content/doc/usage/push-options.en-us.md

Co-authored-by: Yarden Shoham <hrsi88@gmail.com>

yardenshoham · 2023-02-05T11:49:37Z

L-G-T-M but I don't know enough about the codebase to approve. BTW you should update the PR description

yardenshoham · 2023-02-05T11:50:28Z

oops, did not know the bot would do that

a1012112796 · 2023-02-05T14:17:15Z

L-G-T-M but I don't know enough about the codebase to approve. BTW you should update the PR description

#19528 is the reason of this pull requst. I think it will be better solution than #12543 (after pushing mark pull as manually merged)

a1012112796 added the type/enhancement An improvement of existing functionality label Nov 27, 2022

yardenshoham reviewed Nov 27, 2022

View reviewed changes

models/migrations/migrations.go Outdated Show resolved Hide resolved

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

a1012112796 and others added 2 commits November 27, 2022 19:59

Update models/migrations/migrations.go

c661bdf

Co-authored-by: Yarden Shoham <hrsi88@gmail.com>

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

834e62e

yardenshoham reviewed Nov 27, 2022

View reviewed changes

models/issues/pull.go Outdated Show resolved Hide resolved

yardenshoham reviewed Nov 27, 2022

View reviewed changes

routers/private/hook_post_receive.go Outdated Show resolved Hide resolved

a1012112796 and others added 2 commits November 28, 2022 07:57

Apply suggestions from code review

eee691f

Co-authored-by: Yarden Shoham <hrsi88@gmail.com>

fix build

6cf8203

Signed-off-by: a1012112796 <1012112796@qq.com>

yardenshoham added this to the 1.19.0 milestone Nov 28, 2022

a1012112796 added 5 commits November 29, 2022 21:22

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

b0898fc

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

61f0924

fix lint

f83fca2

Signed-off-by: a1012112796 <1012112796@qq.com>

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

286d031

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

b485854

zeripath reviewed Dec 16, 2022

View reviewed changes

a1012112796 added 5 commits December 16, 2022 15:01

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

335a2e6

limit base_branch

0b3ef59

Signed-off-by: a1012112796 <1012112796@qq.com>

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

4b7f007

add ManuallyMergeConfirmedVersion to sync ManuallyMergeConfirmed

b8ffb32

Signed-off-by: a1012112796 <1012112796@qq.com>

fix lint

1997f8c

Signed-off-by: a1012112796 <1012112796@qq.com>

a1012112796 added 2 commits February 4, 2023 11:40

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

7e0aa22

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

ffd62b6

a1012112796 requested review from zeripath and yardenshoham February 4, 2023 13:53

yardenshoham reviewed Feb 4, 2023

View reviewed changes

rename push option

e4c3fb9

Signed-off-by: a1012112796 <1012112796@qq.com>

update document

3b682a8

Signed-off-by: a1012112796 <1012112796@qq.com>

yardenshoham reviewed Feb 5, 2023

View reviewed changes

docs/content/doc/usage/push-options.en-us.md Outdated Show resolved Hide resolved

Update docs/content/doc/usage/push-options.en-us.md

7bc20a7

Co-authored-by: Yarden Shoham <hrsi88@gmail.com>

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 Feb 5, 2023

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 5, 2023

a1012112796 removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 5, 2023

Merge branch 'main' into zzc/dev/merge-pull-manually-quick

61f2dd4

yardenshoham modified the milestones: 1.19.0, 1.20.0 Feb 22, 2023

delvh removed this from the 1.20.0 milestone Jun 4, 2023

a1012112796 closed this Nov 6, 2023

a1012112796 deleted the zzc/dev/merge-pull-manually-quick branch November 6, 2023 01:42

go-gitea locked as resolved and limited conversation to collaborators Feb 4, 2024

Uh oh!

allow mark PR as manually merged by push option #21951

allow mark PR as manually merged by push option #21951

Uh oh!

Conversation

a1012112796 commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeripath Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

a1012112796 Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeripath Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

a1012112796 Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

a1012112796 Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

zeripath Dec 18, 2022

Choose a reason for hiding this comment

Uh oh!

a1012112796 Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a1012112796 Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lunny commented Feb 3, 2023

Uh oh!

yardenshoham Feb 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yardenshoham commented Feb 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yardenshoham commented Feb 5, 2023

Uh oh!

a1012112796 commented Feb 5, 2023

Uh oh!

Uh oh!

a1012112796 commented Nov 27, 2022 •

edited

Loading

a1012112796 Dec 16, 2022 •

edited

Loading

a1012112796 Dec 19, 2022 •

edited

Loading

codecov-commenter commented Dec 20, 2022 •

edited

Loading

yardenshoham commented Feb 5, 2023 •

edited

Loading