Skip to content
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

Agit flow add refs/for-review/<pull index> support #33179

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

hiifong
Copy link
Member

@hiifong hiifong commented Jan 9, 2025

The original PR is #31245. Thanks to @a1012112796 for everything you did for this pr. Since I haven't heard from the author for a long time, I decided to try to resolve the conflict.

How to update a pull request using agit?

git push origin HEAD:refs/for-review/{pull_request_index}

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 9, 2025
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 9, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/templates This PR modifies the template files labels Jan 9, 2025
@hiifong hiifong marked this pull request as draft January 9, 2025 14:08
@yp05327 yp05327 changed the title agit flow add refs/for-review/<pull index> support Agit flow add refs/for-review/<pull index> support Jan 10, 2025
@hiifong hiifong marked this pull request as ready for review January 11, 2025 07:50
@hiifong
Copy link
Member Author

hiifong commented Jan 11, 2025

This PR is ready for review.

@lunny lunny added this to the 1.24.0 milestone Jan 11, 2025
services/pull/edits.go Outdated Show resolved Hide resolved
return fmt.Errorf("Internal Server Error (no specific error)")
}

if ctx.userPerm.CanWrite(unit.TypeCode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user with read permission to the code should be allowed to create/update pull requests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating pull requests requires write permission.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating pull requests needs write permission of the head repository.

@@ -236,7 +236,8 @@ func runServ(c *cli.Context) error {
if git.DefaultFeatures().SupportProcReceive {
// for AGit Flow
if cmd == "ssh_info" {
fmt.Print(`{"type":"gitea","version":1}`)
data := private.GetSSHInfo(ctx)
fmt.Println(data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "data" is always hard-coded string {"type":"gitea","version":2}? Why it needs a http request

}

if !ctx.loadPusherAndPermission() {
return fmt.Errorf("Internal Server Error (no specific error)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message seems not informative.


if err := canUpdateAgitPull(ctx, pull); err != nil {
ctx.JSON(http.StatusForbidden, private.Response{
UserMsg: err.Error(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid putting err.Error() to UserMsag. At the moment, these messages are written by you, but in the future, developers might fill server-side sensitive information into it.

@@ -46,6 +125,49 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
continue
}

if opts.RefFullNames[i].IsForReview() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This for-loop is too large to maintain. I think we need to split it into small functions (and it's easier to use unit test to cover the small functions)

@@ -1,5 +1,5 @@
{{if and .Issue.IsPull .IsIssuePoster (not .Issue.IsClosed) .Issue.PullRequest.HeadRepo}}
{{if and (not (eq .Issue.PullRequest.HeadRepo.FullName .Issue.PullRequest.BaseRepo.FullName)) .CanWriteToHeadRepo}}
{{if or (eq .SignedUserID .Issue.PosterID) (and (not (eq .Issue.PullRequest.HeadRepo.FullName .Issue.PullRequest.BaseRepo.FullName)) .CanWriteToHeadRepo)}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anyone able to really understand the complex logic here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants