sususweet
opened 1 year ago
-
Previous Value Current Value feat: Able to synchronize source branch with when processing a pull request.
feat: Able to synchronize source branch when processing a pull request.
-
Thanks for submitting the pull request. However there are some points to consider:
- Permission check: simply check write code permission of the project is not enough, as there might exist branch protection rules for source branch. Check branch update logic via push here: https://code.onedev.io/onedev/server/~files/0b26cd185d2e6b386b707bae8061ac2ce78f179d/server-core/src/main/java/io/onedev/server/git/hook/GitPreReceiveCallback.java?position=source-133.1-167.7-1
- Rebase should also be allowed to update source branch in addition to merge. And in case of merge, user should be allowed to input commit message, and the commit message should be checked against branch protection rule to see if it meets the commit message requirement. An example here: https://code.onedev.io/onedev/server/~files/0b26cd185d2e6b386b707bae8061ac2ce78f179d/server-core/src/main/java/io/onedev/server/web/page/project/pullrequests/detail/PullRequestDetailPage.java?position=source-1688.1-1710.7-1
Also branch protection rule check can be slow (may need to calculate changed files) and better to be put inside checkAsync and set some flag in pull request to speed up frontend rendering.
Since this task is a bit complex and quite a few things need to be considered, I plan to work on this feature at my side. I do appreciate your help on improving OneDev, but this task might not be suitable for starters. Also for future contributions, we may discuss first before working on the code.
-
Thanks for submitting the pull request. However there are some points to consider:
- Permission check: simply check write code permission of the project is not enough, as there might exist branch protection rules for source branch. Check branch update logic via push here: https://code.onedev.io/onedev/server/~files/0b26cd185d2e6b386b707bae8061ac2ce78f179d/server-core/src/main/java/io/onedev/server/git/hook/GitPreReceiveCallback.java?position=source-133.1-167.7-1
- Rebase should also be allowed to update source branch in addition to merge. And in case of merge, user should be allowed to input commit message, and the commit message should be checked against branch protection rule to see if it meets the commit message requirement. An example here: https://code.onedev.io/onedev/server/~files/0b26cd185d2e6b386b707bae8061ac2ce78f179d/server-core/src/main/java/io/onedev/server/web/page/project/pullrequests/detail/PullRequestDetailPage.java?position=source-1688.1-1710.7-1
Also branch protection rule check can be slow (may need to calculate changed files) and better to be put inside checkAsync and set some flag in pull request to speed up frontend rendering.
Since this task is a bit complex and quite a few things need to be considered, I plan to work on this feature at my side. I do appreciate your help on improving OneDev, but this task might not be suitable for starters. Also for future contributions, we may discuss first before working on the code.
Thanks for replying. I do really want to do something for OneDev's improvement. If possible, I would like to work on this pr and realize the requirements mentioned above. I wonder if you could give me the chance to contribute to OneDev.
-
Sure. Please go ahead to improve this PR.
Also branch protection rule check can be slow (may need to calculate changed files) and better to be put inside checkAsync and set some flag in pull request to speed up frontend rendering
We can leave this alone for now to make things easier first.
-
Sure. Please go ahead to improve this PR.
Also branch protection rule check can be slow (may need to calculate changed files) and better to be put inside checkAsync and set some flag in pull request to speed up frontend rendering
We can leave this alone for now to make things easier first.
- Add Permission check for branch protection rules of source branch.
- Add Rebase method to update source branch.
- Add commit message for user's input.
These features have been added to this pr. Wish you could review this pr and give some advice.
-
Problems found:
- Even if rebase option is selected, source branch is still using the merge strategy. This is because you are storing the update strategy in transient field of pull request, and pull request object will be reloaded for each web request (actually almost all hibernate entities are reloaded for each web request, to avoid potential LazyLoadException). Since update source branch is achieved within a single operation, the update strategy should be passed as a param of updateSourceBranch instead of storing as a field of PullRequest).
- For both update strategy, messages of all commits being merged into source branch should also be checked. This method demonstrates how to check commits merged from source branch
- For both update strategy, signature of head commit of target branch should also be checked. This method demonstrates how to check head commit of source branch
- For both update strategy, build requirement of should also be checked. Refer to this example
- For update strategy, user does not need to input commit message. Just display a dialog to ask for confirmation
Usability suggestions:
- Style of "update source branch" button should be the same as "delete source branch" and "restore source branch" button, and it should be placed before "delete source branch" button
- For message "Source branch is 4 commit(s) ahead of target branch, 1 commit(s) behind of target branch" (an example), remove the part "4 commit(s) ahead of target branch" as this message is redundant for a pull request. For the part "1 commit(s) behind of target branch", display a link to RevisionComparePage to show actual commits so that user can easily know what commits will be synced to the source branch.
-
Problems found:
- Even if rebase option is selected, source branch is still using the merge strategy. This is because you are storing the update strategy in transient field of pull request, and pull request object will be reloaded for each web request (actually almost all hibernate entities are reloaded for each web request, to avoid potential LazyLoadException). Since update source branch is achieved within a single operation, the update strategy should be passed as a param of updateSourceBranch instead of storing as a field of PullRequest).
- For both update strategy, messages of all commits being merged into source branch should also be checked. This method demonstrates how to check commits merged from source branch
- For both update strategy, signature of head commit of target branch should also be checked. This method demonstrates how to check head commit of source branch
- For both update strategy, build requirement of should also be checked. Refer to this example
- For update strategy, user does not need to input commit message. Just display a dialog to ask for confirmation
Usability suggestions:
- Style of "update source branch" button should be the same as "delete source branch" and "restore source branch" button, and it should be placed before "delete source branch" button
- For message "Source branch is 4 commit(s) ahead of target branch, 1 commit(s) behind of target branch" (an example), remove the part "4 commit(s) ahead of target branch" as this message is redundant for a pull request. For the part "1 commit(s) behind of target branch", display a link to RevisionComparePage to show actual commits so that user can easily know what commits will be synced to the source branch.
These advices have been added to this pr. Wish you could review this pr and give some advice.
-
Problems found:
- User should be able to specify commit message if select the merge strategy. For rebase strategy, just display the confirmation dialog as now.
- The branch protection rules should be checked right after user selects to update the branch, and any violation messages should be displayed before asking user to confirm the operation. Current approach is to ask user to confirm first, and then check the branch protection rule which is sub-optimal.
-
Problems found:
- User should be able to specify commit message if select the merge strategy. For rebase strategy, just display the confirmation dialog as now.
- The branch protection rules should be checked right after user selects to update the branch, and any violation messages should be displayed before asking user to confirm the operation. Current approach is to ask user to confirm first, and then check the branch protection rule which is sub-optimal.
These advices have been added to this pr. Wish you could review this pr and give some advice.
-
-
-
Thank you for the effort!
-
Thank you for the effort!
My pleasure. Hope to contribute to OneDev in the future.
-
I reworked this feature via commit d8ccfc85. Some reasons:
- Previous implementation can not handle the case when source project is different from target project
- Many unnecessary code in
PullRequestManager.updateSourceBranch, and some of them can lead to incorrect behaviors. - Calculating AheadBehind multiple times in a single request which is inefficient. Actually we only need to check if source branch is outdated
- Calculating merge commits multiple times which can be slow. Now once a merge commit is calculated in PullRequestDetailPage.checkUpdateSourceBranch, it will be reused later to update source branch (only need to amend it with user supplied commit message for merge strategy)
- Use existing utilities such as MenuLink, ConfirmModalPanel, BeanEditModalPanel to avoid code duplication
Hope this can help you understand OneDev code base better to make future contributions more efficient.
