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

Implement rename branch API #32433

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kemzeb
Copy link
Contributor

@kemzeb kemzeb commented Nov 6, 2024

Resolves #22526.

Builds upon #23061.

Co-authored-by: sillyguodong <[email protected]>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 6, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 6, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Nov 6, 2024
@@ -396,6 +396,95 @@ func ListBranches(ctx *context.APIContext) {
ctx.JSON(http.StatusOK, apiBranches)
}

// RenameBranch renames a repository's branch.
func RenameBranch(ctx *context.APIContext) {
// swagger:operation POST /repos/{owner}/{repo}/branches/{name}/rename repository repoRenameBranch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous PR did have unresolved feedback here.

If I understand correctly, the question is if having a branch "test/branch/rename" would cause our http routing to instead look for "test/branch"? If this was the question, this shouldn't be the case. The endpoint would like the following (but URL-encoded):

/api/v1/:owner/:repo/branches/test/branch/rename/rename

So this should be fine. From manually testing with our swagger client, this appears to be case.

Copy link
Member

Choose a reason for hiding this comment

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

@silverwind silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 6, 2024
@@ -1195,6 +1195,7 @@ func Routes() *web.Router {
m.Get("/*", repo.GetBranch)
m.Delete("/*", reqToken(), reqRepoWriter(unit.TypeCode), mustNotBeArchived, repo.DeleteBranch)
m.Post("", reqToken(), reqRepoWriter(unit.TypeCode), mustNotBeArchived, bind(api.CreateBranchRepoOption{}), repo.CreateBranch)
m.Post("/{name}/rename", tokenRequiresScopes(auth_model.AccessTokenScopeCategoryRepository), reqRepoWriter(unit.TypeCode), bind(api.RenameBranchRepoOption{}), repo.RenameBranch)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe /*/rename can work? branch name maybe include /.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I am missing something here, but wouldn't we expect {name} to be URL-encoded? This makes sense to me as it should avoid the http router from misinterpreting the path segments. The swagger client does URL-encode test/branch/rename when I make a request. Is this not something we expect clients to do?

Copy link
Member

Choose a reason for hiding this comment

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

if {name} is URl-encoded, why not put them into the post key/value?

Copy link
Contributor

@wxiaoguang wxiaoguang Nov 7, 2024

Choose a reason for hiding this comment

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

If you are trying to follow GitHub, they do not need to encode that path (If I understand correctly)

And none of our handlers above requires to encode the "path".

OR: do not follow github, provide the names in request body, then everything is clear.

image

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/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Api for branch rename
5 participants