feat: adds option to force overwrite new branch for /contents route (#12663)
Adds an option "force_overwrite_new_branch" when posting to
/repos/{owner}/{repo}/contents to modify multiple files in a repository.
When user provides both "branch" and "new_branch" options, and
"new_branch" already exists, the "force_overwrite_new_branch" option
allows the user to overwrite the existing branch. Under the hood this
amounts to a "git push --force".
[Issue #12600](https://codeberg.org/forgejo/forgejo/issues/12600)
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [x] `make pr-go` before pushing
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [ ] I did not document these changes and I do not expect someone else to do it.
- [x] API swagger docs updated
### Release notes
- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.
Co-authored-by: Rob Gonnella <rob.gonnella@papayapay.com>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12663
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
This commit is contained in:
committed by
Mathieu Fenniak
co-authored by
Rob Gonnella
parent
b1b47e64d7
commit
c956dae73d
@@ -135,3 +135,6 @@ prime/
|
|||||||
# Manpage
|
# Manpage
|
||||||
/man
|
/man
|
||||||
tests/integration/api_activitypub_person_inbox_useractivity_test.go
|
tests/integration/api_activitypub_person_inbox_useractivity_test.go
|
||||||
|
|
||||||
|
# Mise version management
|
||||||
|
mise.toml
|
||||||
|
|||||||
@@ -20,6 +20,8 @@ type FileOptions struct {
|
|||||||
Dates CommitDateOptions `json:"dates"`
|
Dates CommitDateOptions `json:"dates"`
|
||||||
// Add a Signed-off-by trailer by the committer at the end of the commit log message.
|
// Add a Signed-off-by trailer by the committer at the end of the commit log message.
|
||||||
Signoff bool `json:"signoff"`
|
Signoff bool `json:"signoff"`
|
||||||
|
// (optional) will do a force-push if the new branch already exists
|
||||||
|
ForceOverwriteNewBranch bool `json:"force_overwrite_new_branch"`
|
||||||
}
|
}
|
||||||
|
|
||||||
// CreateFileOptions options for creating files
|
// CreateFileOptions options for creating files
|
||||||
|
|||||||
@@ -533,7 +533,8 @@ func ChangeFiles(ctx *context.APIContext) {
|
|||||||
Author: apiOpts.Dates.Author,
|
Author: apiOpts.Dates.Author,
|
||||||
Committer: apiOpts.Dates.Committer,
|
Committer: apiOpts.Dates.Committer,
|
||||||
},
|
},
|
||||||
Signoff: apiOpts.Signoff,
|
Signoff: apiOpts.Signoff,
|
||||||
|
ForceOverwriteNewBranch: apiOpts.ForceOverwriteNewBranch,
|
||||||
}
|
}
|
||||||
if opts.Dates.Author.IsZero() {
|
if opts.Dates.Author.IsZero() {
|
||||||
opts.Dates.Author = time.Now()
|
opts.Dates.Author = time.Now()
|
||||||
@@ -634,7 +635,8 @@ func CreateFile(ctx *context.APIContext) {
|
|||||||
Author: apiOpts.Dates.Author,
|
Author: apiOpts.Dates.Author,
|
||||||
Committer: apiOpts.Dates.Committer,
|
Committer: apiOpts.Dates.Committer,
|
||||||
},
|
},
|
||||||
Signoff: apiOpts.Signoff,
|
Signoff: apiOpts.Signoff,
|
||||||
|
ForceOverwriteNewBranch: apiOpts.ForceOverwriteNewBranch,
|
||||||
}
|
}
|
||||||
if opts.Dates.Author.IsZero() {
|
if opts.Dates.Author.IsZero() {
|
||||||
opts.Dates.Author = time.Now()
|
opts.Dates.Author = time.Now()
|
||||||
@@ -741,7 +743,8 @@ func UpdateFile(ctx *context.APIContext) {
|
|||||||
Author: apiOpts.Dates.Author,
|
Author: apiOpts.Dates.Author,
|
||||||
Committer: apiOpts.Dates.Committer,
|
Committer: apiOpts.Dates.Committer,
|
||||||
},
|
},
|
||||||
Signoff: apiOpts.Signoff,
|
Signoff: apiOpts.Signoff,
|
||||||
|
ForceOverwriteNewBranch: apiOpts.ForceOverwriteNewBranch,
|
||||||
}
|
}
|
||||||
if opts.Dates.Author.IsZero() {
|
if opts.Dates.Author.IsZero() {
|
||||||
opts.Dates.Author = time.Now()
|
opts.Dates.Author = time.Now()
|
||||||
@@ -767,6 +770,12 @@ func handleCreateOrUpdateFileError(ctx *context.APIContext, err error) {
|
|||||||
ctx.Error(http.StatusForbidden, "Access", err)
|
ctx.Error(http.StatusForbidden, "Access", err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if git.IsErrPushRejected(err) {
|
||||||
|
rejectErr := err.(*git.ErrPushRejected)
|
||||||
|
rejectErr.GenerateMessage()
|
||||||
|
ctx.Error(http.StatusForbidden, "PushRejected", rejectErr)
|
||||||
|
return
|
||||||
|
}
|
||||||
if git_model.IsErrBranchAlreadyExists(err) ||
|
if git_model.IsErrBranchAlreadyExists(err) ||
|
||||||
models.IsErrFilenameInvalid(err) ||
|
models.IsErrFilenameInvalid(err) ||
|
||||||
models.IsErrSHAOrCommitIDNotProvided(err) ||
|
models.IsErrSHAOrCommitIDNotProvided(err) ||
|
||||||
@@ -910,7 +919,8 @@ func DeleteFile(ctx *context.APIContext) {
|
|||||||
Author: apiOpts.Dates.Author,
|
Author: apiOpts.Dates.Author,
|
||||||
Committer: apiOpts.Dates.Committer,
|
Committer: apiOpts.Dates.Committer,
|
||||||
},
|
},
|
||||||
Signoff: apiOpts.Signoff,
|
Signoff: apiOpts.Signoff,
|
||||||
|
ForceOverwriteNewBranch: apiOpts.ForceOverwriteNewBranch,
|
||||||
}
|
}
|
||||||
if opts.Dates.Author.IsZero() {
|
if opts.Dates.Author.IsZero() {
|
||||||
opts.Dates.Author = time.Now()
|
opts.Dates.Author = time.Now()
|
||||||
|
|||||||
@@ -302,7 +302,7 @@ func alterRepositoryContent(ctx context.Context, doer *user_model.User, repo *re
|
|||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
return t.Push(doer, commitHash, repo.DefaultBranch)
|
return t.Push(doer, commitHash, repo.DefaultBranch, false)
|
||||||
}
|
}
|
||||||
|
|
||||||
func writeObjectToIndex(t *files_service.TemporaryUploadRepository, path string, r io.Reader) error {
|
func writeObjectToIndex(t *files_service.TemporaryUploadRepository, path string, r io.Reader) error {
|
||||||
|
|||||||
@@ -121,7 +121,7 @@ func CherryPick(ctx context.Context, repo *repo_model.Repository, doer *user_mod
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Then push this tree to NewBranch
|
// Then push this tree to NewBranch
|
||||||
if err := t.Push(doer, commitHash, opts.NewBranch); err != nil {
|
if err := t.Push(doer, commitHash, opts.NewBranch, false); err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -175,7 +175,7 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Then push this tree to NewBranch
|
// Then push this tree to NewBranch
|
||||||
if err := t.Push(doer, commitHash, opts.NewBranch); err != nil {
|
if err := t.Push(doer, commitHash, opts.NewBranch, false); err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -298,12 +298,13 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(parent string, author, co
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Push the provided commitHash to the repository branch by the provided user
|
// Push the provided commitHash to the repository branch by the provided user
|
||||||
func (t *TemporaryUploadRepository) Push(doer *user_model.User, commitHash, branch string) error {
|
func (t *TemporaryUploadRepository) Push(doer *user_model.User, commitHash, branch string, force bool) error {
|
||||||
// Because calls hooks we need to pass in the environment
|
// Because calls hooks we need to pass in the environment
|
||||||
env := repo_module.PushingEnvironment(doer, t.repo)
|
env := repo_module.PushingEnvironment(doer, t.repo)
|
||||||
if err := git.Push(t.ctx, t.basePath, git.PushOptions{
|
if err := git.Push(t.ctx, t.basePath, git.PushOptions{
|
||||||
Remote: t.repo.RepoPath(),
|
Remote: t.repo.RepoPath(),
|
||||||
Branch: strings.TrimSpace(commitHash) + ":" + git.BranchPrefix + strings.TrimSpace(branch),
|
Branch: strings.TrimSpace(commitHash) + ":" + git.BranchPrefix + strings.TrimSpace(branch),
|
||||||
|
Force: force,
|
||||||
Env: env,
|
Env: env,
|
||||||
}); err != nil {
|
}); err != nil {
|
||||||
if git.IsErrPushOutOfDate(err) {
|
if git.IsErrPushOutOfDate(err) {
|
||||||
|
|||||||
@@ -48,15 +48,16 @@ type ChangeRepoFile struct {
|
|||||||
|
|
||||||
// ChangeRepoFilesOptions holds the repository files update options
|
// ChangeRepoFilesOptions holds the repository files update options
|
||||||
type ChangeRepoFilesOptions struct {
|
type ChangeRepoFilesOptions struct {
|
||||||
LastCommitID string
|
LastCommitID string
|
||||||
OldBranch string
|
OldBranch string
|
||||||
NewBranch string
|
NewBranch string
|
||||||
Message string
|
Message string
|
||||||
Files []*ChangeRepoFile
|
Files []*ChangeRepoFile
|
||||||
Author *IdentityOptions
|
Author *IdentityOptions
|
||||||
Committer *IdentityOptions
|
Committer *IdentityOptions
|
||||||
Dates *CommitDateOptions
|
Dates *CommitDateOptions
|
||||||
Signoff bool
|
Signoff bool
|
||||||
|
ForceOverwriteNewBranch bool
|
||||||
}
|
}
|
||||||
|
|
||||||
type RepoFileOptions struct {
|
type RepoFileOptions struct {
|
||||||
@@ -136,7 +137,7 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
|
|||||||
// If we aren't branching to a new branch, make sure user can commit to the given branch
|
// If we aren't branching to a new branch, make sure user can commit to the given branch
|
||||||
if opts.NewBranch != opts.OldBranch {
|
if opts.NewBranch != opts.OldBranch {
|
||||||
existingBranch, err := gitRepo.GetBranch(opts.NewBranch)
|
existingBranch, err := gitRepo.GetBranch(opts.NewBranch)
|
||||||
if existingBranch != nil {
|
if existingBranch != nil && !opts.ForceOverwriteNewBranch {
|
||||||
return nil, git_model.ErrBranchAlreadyExists{
|
return nil, git_model.ErrBranchAlreadyExists{
|
||||||
BranchName: opts.NewBranch,
|
BranchName: opts.NewBranch,
|
||||||
}
|
}
|
||||||
@@ -263,7 +264,7 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Then push this tree to NewBranch
|
// Then push this tree to NewBranch
|
||||||
if err := t.Push(doer, commitHash, opts.NewBranch); err != nil {
|
if err := t.Push(doer, commitHash, opts.NewBranch, opts.ForceOverwriteNewBranch); err != nil {
|
||||||
log.Error("%T %v", err, err)
|
log.Error("%T %v", err, err)
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -158,7 +158,7 @@ func UploadRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Then push this tree to NewBranch
|
// Then push this tree to NewBranch
|
||||||
if err := t.Push(doer, commitHash, opts.NewBranch); err != nil {
|
if err := t.Push(doer, commitHash, opts.NewBranch, false); err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Generated
+20
@@ -23921,6 +23921,11 @@
|
|||||||
},
|
},
|
||||||
"x-go-name": "Files"
|
"x-go-name": "Files"
|
||||||
},
|
},
|
||||||
|
"force_overwrite_new_branch": {
|
||||||
|
"description": "(optional) will do a force-push if the new branch already exists",
|
||||||
|
"type": "boolean",
|
||||||
|
"x-go-name": "ForceOverwriteNewBranch"
|
||||||
|
},
|
||||||
"message": {
|
"message": {
|
||||||
"description": "message (optional) for the commit of this file. if not supplied, a default message will be used",
|
"description": "message (optional) for the commit of this file. if not supplied, a default message will be used",
|
||||||
"type": "string",
|
"type": "string",
|
||||||
@@ -24624,6 +24629,11 @@
|
|||||||
"dates": {
|
"dates": {
|
||||||
"$ref": "#/definitions/CommitDateOptions"
|
"$ref": "#/definitions/CommitDateOptions"
|
||||||
},
|
},
|
||||||
|
"force_overwrite_new_branch": {
|
||||||
|
"description": "(optional) will do a force-push if the new branch already exists",
|
||||||
|
"type": "boolean",
|
||||||
|
"x-go-name": "ForceOverwriteNewBranch"
|
||||||
|
},
|
||||||
"message": {
|
"message": {
|
||||||
"description": "message (optional) for the commit of this file. if not supplied, a default message will be used",
|
"description": "message (optional) for the commit of this file. if not supplied, a default message will be used",
|
||||||
"type": "string",
|
"type": "string",
|
||||||
@@ -25610,6 +25620,11 @@
|
|||||||
"dates": {
|
"dates": {
|
||||||
"$ref": "#/definitions/CommitDateOptions"
|
"$ref": "#/definitions/CommitDateOptions"
|
||||||
},
|
},
|
||||||
|
"force_overwrite_new_branch": {
|
||||||
|
"description": "(optional) will do a force-push if the new branch already exists",
|
||||||
|
"type": "boolean",
|
||||||
|
"x-go-name": "ForceOverwriteNewBranch"
|
||||||
|
},
|
||||||
"message": {
|
"message": {
|
||||||
"description": "message (optional) for the commit of this file. if not supplied, a default message will be used",
|
"description": "message (optional) for the commit of this file. if not supplied, a default message will be used",
|
||||||
"type": "string",
|
"type": "string",
|
||||||
@@ -30610,6 +30625,11 @@
|
|||||||
"dates": {
|
"dates": {
|
||||||
"$ref": "#/definitions/CommitDateOptions"
|
"$ref": "#/definitions/CommitDateOptions"
|
||||||
},
|
},
|
||||||
|
"force_overwrite_new_branch": {
|
||||||
|
"description": "(optional) will do a force-push if the new branch already exists",
|
||||||
|
"type": "boolean",
|
||||||
|
"x-go-name": "ForceOverwriteNewBranch"
|
||||||
|
},
|
||||||
"from_path": {
|
"from_path": {
|
||||||
"description": "from_path (optional) is the path of the original file which will be moved/renamed to the path in the URL",
|
"description": "from_path (optional) is the path of the original file which will be moved/renamed to the path in the URL",
|
||||||
"type": "string",
|
"type": "string",
|
||||||
|
|||||||
+1
-1
@@ -108,5 +108,5 @@ func initRepo(doer *user_model.User, repo *repo_model.Repository, format git.Obj
|
|||||||
return "", err
|
return "", err
|
||||||
}
|
}
|
||||||
|
|
||||||
return commitHash, t.Push(doer, commitHash, repo.DefaultBranch)
|
return commitHash, t.Push(doer, commitHash, repo.DefaultBranch, false)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -310,5 +310,81 @@ func TestAPIChangeFiles(t *testing.T) {
|
|||||||
req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/contents", user2.Name, repo1.Name), &changeFilesOptions).
|
req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/contents", user2.Name, repo1.Name), &changeFilesOptions).
|
||||||
AddTokenAuth(token4)
|
AddTokenAuth(token4)
|
||||||
MakeRequest(t, req, http.StatusForbidden)
|
MakeRequest(t, req, http.StatusForbidden)
|
||||||
|
|
||||||
|
// Test fails creating files in a branch that already exists without force push
|
||||||
|
changeFilesOptions = getChangeFilesOptions()
|
||||||
|
changeFilesOptions.BranchName = repo1.DefaultBranch
|
||||||
|
changeFilesOptions.NewBranchName = "develop"
|
||||||
|
changeFilesOptions.ForceOverwriteNewBranch = false
|
||||||
|
fileID++
|
||||||
|
createTreePath = fmt.Sprintf("new/file%d.txt", fileID)
|
||||||
|
updateTreePath = fmt.Sprintf("update/file%d.txt", fileID)
|
||||||
|
deleteTreePath = fmt.Sprintf("delete/file%d.txt", fileID)
|
||||||
|
changeFilesOptions.Files[0].Path = createTreePath
|
||||||
|
changeFilesOptions.Files[1].Path = updateTreePath
|
||||||
|
changeFilesOptions.Files[2].Path = deleteTreePath
|
||||||
|
createFile(user2, repo1, updateTreePath)
|
||||||
|
createFile(user2, repo1, deleteTreePath)
|
||||||
|
url = fmt.Sprintf("/api/v1/repos/%s/%s/contents", user2.Name, repo1.Name)
|
||||||
|
req = NewRequestWithJSON(t, "POST", url, &changeFilesOptions).
|
||||||
|
AddTokenAuth(token2)
|
||||||
|
MakeRequest(t, req, http.StatusUnprocessableEntity)
|
||||||
|
|
||||||
|
// Test succeeds creating files in a branch that already exists with force push
|
||||||
|
changeFilesOptions = getChangeFilesOptions()
|
||||||
|
changeFilesOptions.BranchName = repo1.DefaultBranch
|
||||||
|
changeFilesOptions.NewBranchName = "develop"
|
||||||
|
changeFilesOptions.ForceOverwriteNewBranch = true
|
||||||
|
fileID++
|
||||||
|
createTreePath = fmt.Sprintf("new/file%d.txt", fileID)
|
||||||
|
updateTreePath = fmt.Sprintf("update/file%d.txt", fileID)
|
||||||
|
deleteTreePath = fmt.Sprintf("delete/file%d.txt", fileID)
|
||||||
|
changeFilesOptions.Files[0].Path = createTreePath
|
||||||
|
changeFilesOptions.Files[1].Path = updateTreePath
|
||||||
|
changeFilesOptions.Files[2].Path = deleteTreePath
|
||||||
|
createFile(user2, repo1, updateTreePath)
|
||||||
|
createFile(user2, repo1, deleteTreePath)
|
||||||
|
url = fmt.Sprintf("/api/v1/repos/%s/%s/contents", user2.Name, repo1.Name)
|
||||||
|
req = NewRequestWithJSON(t, "POST", url, &changeFilesOptions).
|
||||||
|
AddTokenAuth(token2)
|
||||||
|
resp = MakeRequest(t, req, http.StatusCreated)
|
||||||
|
DecodeJSON(t, resp, &filesResponse)
|
||||||
|
expectedCreateHTMLURL = fmt.Sprintf(setting.AppURL+"user2/repo1/src/branch/develop/new/file%d.txt", fileID)
|
||||||
|
expectedCreateDownloadURL = fmt.Sprintf(setting.AppURL+"user2/repo1/raw/branch/develop/new/file%d.txt", fileID)
|
||||||
|
expectedUpdateHTMLURL = fmt.Sprintf(setting.AppURL+"user2/repo1/src/branch/develop/update/file%d.txt", fileID)
|
||||||
|
expectedUpdateDownloadURL = fmt.Sprintf(setting.AppURL+"user2/repo1/raw/branch/develop/update/file%d.txt", fileID)
|
||||||
|
assert.Equal(t, expectedCreateSHA, filesResponse.Files[0].SHA)
|
||||||
|
assert.Equal(t, expectedCreateHTMLURL, *filesResponse.Files[0].HTMLURL)
|
||||||
|
assert.Equal(t, expectedCreateDownloadURL, *filesResponse.Files[0].DownloadURL)
|
||||||
|
assert.Equal(t, expectedUpdateSHA, filesResponse.Files[1].SHA)
|
||||||
|
assert.Equal(t, expectedUpdateHTMLURL, *filesResponse.Files[1].HTMLURL)
|
||||||
|
assert.Equal(t, expectedUpdateDownloadURL, *filesResponse.Files[1].DownloadURL)
|
||||||
|
assert.Nil(t, filesResponse.Files[2])
|
||||||
|
assert.Equal(t, changeFilesOptions.Message+"\n", filesResponse.Commit.Message)
|
||||||
|
|
||||||
|
// Test fails creating files in a branch that already exists with force push and branch protection
|
||||||
|
protectionReq := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branch_protections", &api.BranchProtection{
|
||||||
|
RuleName: "develop",
|
||||||
|
BranchName: "develop",
|
||||||
|
EnablePush: true,
|
||||||
|
}).AddTokenAuth(token2)
|
||||||
|
MakeRequest(t, protectionReq, http.StatusCreated)
|
||||||
|
changeFilesOptions = getChangeFilesOptions()
|
||||||
|
changeFilesOptions.BranchName = repo1.DefaultBranch
|
||||||
|
changeFilesOptions.NewBranchName = "develop"
|
||||||
|
changeFilesOptions.ForceOverwriteNewBranch = true
|
||||||
|
fileID++
|
||||||
|
createTreePath = fmt.Sprintf("new/file%d.txt", fileID)
|
||||||
|
updateTreePath = fmt.Sprintf("update/file%d.txt", fileID)
|
||||||
|
deleteTreePath = fmt.Sprintf("delete/file%d.txt", fileID)
|
||||||
|
changeFilesOptions.Files[0].Path = createTreePath
|
||||||
|
changeFilesOptions.Files[1].Path = updateTreePath
|
||||||
|
changeFilesOptions.Files[2].Path = deleteTreePath
|
||||||
|
createFile(user2, repo1, updateTreePath)
|
||||||
|
createFile(user2, repo1, deleteTreePath)
|
||||||
|
url = fmt.Sprintf("/api/v1/repos/%s/%s/contents", user2.Name, repo1.Name)
|
||||||
|
req = NewRequestWithJSON(t, "POST", url, &changeFilesOptions).
|
||||||
|
AddTokenAuth(token2)
|
||||||
|
MakeRequest(t, req, http.StatusForbidden)
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user