[REFACTOR] Refactor the AGit code
TLDR: Less code, better maintainability and more comments. - Add code comments to explain what the code does, it's quite a big function so it definitely deserved some of that. - Simplify some logic. - Load the `pusher` in a single place. - Update the error messages to be more correct, not capitlized, include more debug info and remove 'Error:' As it's no need to indicate that, errors are concenated with `:` seperators. - Improve the message that a change was rejected, because a force push was detected and the `force-push` option wasn't set. - Avoid a second time loading `gitRepo.GetObjectFormat` and handle the error gracefully for the other occurence. - Adds integration test for force push detection.
This commit is contained in:
parent
5240e27266
commit
c58ad87513
|
@ -21,37 +21,36 @@ import (
|
||||||
|
|
||||||
// ProcReceive handle proc receive work
|
// ProcReceive handle proc receive work
|
||||||
func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, opts *private.HookOptions) ([]private.HookProcReceiveRefResult, error) {
|
func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, opts *private.HookOptions) ([]private.HookProcReceiveRefResult, error) {
|
||||||
// TODO: Add more options?
|
|
||||||
var (
|
|
||||||
topicBranch string
|
|
||||||
title string
|
|
||||||
description string
|
|
||||||
forcePush bool
|
|
||||||
)
|
|
||||||
|
|
||||||
results := make([]private.HookProcReceiveRefResult, 0, len(opts.OldCommitIDs))
|
results := make([]private.HookProcReceiveRefResult, 0, len(opts.OldCommitIDs))
|
||||||
|
|
||||||
ownerName := repo.OwnerName
|
topicBranch := opts.GitPushOptions["topic"]
|
||||||
repoName := repo.Name
|
_, forcePush := opts.GitPushOptions["force-push"]
|
||||||
|
|
||||||
topicBranch = opts.GitPushOptions["topic"]
|
|
||||||
_, forcePush = opts.GitPushOptions["force-push"]
|
|
||||||
objectFormat, _ := gitRepo.GetObjectFormat()
|
|
||||||
|
|
||||||
title, hasTitle := opts.GitPushOptions["title"]
|
title, hasTitle := opts.GitPushOptions["title"]
|
||||||
description, hasDesc := opts.GitPushOptions["description"]
|
description, hasDesc := opts.GitPushOptions["description"]
|
||||||
|
|
||||||
|
objectFormat, err := gitRepo.GetObjectFormat()
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("couldn't get object format of the repository: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
pusher, err := user_model.GetUserByID(ctx, opts.UserID)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("failed to get user[%d]: %w", opts.UserID, err)
|
||||||
|
}
|
||||||
|
|
||||||
for i := range opts.OldCommitIDs {
|
for i := range opts.OldCommitIDs {
|
||||||
|
// Avoid processing this change if the new commit is empty.
|
||||||
if opts.NewCommitIDs[i] == objectFormat.EmptyObjectID().String() {
|
if opts.NewCommitIDs[i] == objectFormat.EmptyObjectID().String() {
|
||||||
results = append(results, private.HookProcReceiveRefResult{
|
results = append(results, private.HookProcReceiveRefResult{
|
||||||
OriginalRef: opts.RefFullNames[i],
|
OriginalRef: opts.RefFullNames[i],
|
||||||
OldOID: opts.OldCommitIDs[i],
|
OldOID: opts.OldCommitIDs[i],
|
||||||
NewOID: opts.NewCommitIDs[i],
|
NewOID: opts.NewCommitIDs[i],
|
||||||
Err: "Can't delete not exist branch",
|
Err: "Cannot delete a non-existent branch.",
|
||||||
})
|
})
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Only process references that are in the form of refs/for/
|
||||||
if !opts.RefFullNames[i].IsFor() {
|
if !opts.RefFullNames[i].IsFor() {
|
||||||
results = append(results, private.HookProcReceiveRefResult{
|
results = append(results, private.HookProcReceiveRefResult{
|
||||||
IsNotMatched: true,
|
IsNotMatched: true,
|
||||||
|
@ -60,10 +59,14 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Get the anything after the refs/for/ prefix.
|
||||||
baseBranchName := opts.RefFullNames[i].ForBranchName()
|
baseBranchName := opts.RefFullNames[i].ForBranchName()
|
||||||
curentTopicBranch := ""
|
curentTopicBranch := topicBranch
|
||||||
|
|
||||||
|
// If the reference was given in the format of refs/for/<target-branch>/<topic-branch>,
|
||||||
|
// where <target-branch> and <topic-branch> can contain slashes, we need to iteratively
|
||||||
|
// search for what the target and topic branch is.
|
||||||
if !gitRepo.IsBranchExist(baseBranchName) {
|
if !gitRepo.IsBranchExist(baseBranchName) {
|
||||||
// try match refs/for/<target-branch>/<topic-branch>
|
|
||||||
for p, v := range baseBranchName {
|
for p, v := range baseBranchName {
|
||||||
if v == '/' && gitRepo.IsBranchExist(baseBranchName[:p]) && p != len(baseBranchName)-1 {
|
if v == '/' && gitRepo.IsBranchExist(baseBranchName[:p]) && p != len(baseBranchName)-1 {
|
||||||
curentTopicBranch = baseBranchName[p+1:]
|
curentTopicBranch = baseBranchName[p+1:]
|
||||||
|
@ -73,46 +76,39 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if len(topicBranch) == 0 && len(curentTopicBranch) == 0 {
|
if len(curentTopicBranch) == 0 {
|
||||||
results = append(results, private.HookProcReceiveRefResult{
|
results = append(results, private.HookProcReceiveRefResult{
|
||||||
OriginalRef: opts.RefFullNames[i],
|
OriginalRef: opts.RefFullNames[i],
|
||||||
OldOID: opts.OldCommitIDs[i],
|
OldOID: opts.OldCommitIDs[i],
|
||||||
NewOID: opts.NewCommitIDs[i],
|
NewOID: opts.NewCommitIDs[i],
|
||||||
Err: "topic-branch is not set",
|
Err: "The topic-branch option is not set",
|
||||||
})
|
})
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
var headBranch string
|
// Include the user's name in the head branch, to avoid conflicts
|
||||||
|
// with other users.
|
||||||
|
headBranch := curentTopicBranch
|
||||||
userName := strings.ToLower(opts.UserName)
|
userName := strings.ToLower(opts.UserName)
|
||||||
|
|
||||||
if len(curentTopicBranch) == 0 {
|
|
||||||
curentTopicBranch = topicBranch
|
|
||||||
}
|
|
||||||
|
|
||||||
// because different user maybe want to use same topic,
|
|
||||||
// So it's better to make sure the topic branch name
|
|
||||||
// has user name prefix
|
|
||||||
if !strings.HasPrefix(curentTopicBranch, userName+"/") {
|
if !strings.HasPrefix(curentTopicBranch, userName+"/") {
|
||||||
headBranch = userName + "/" + curentTopicBranch
|
headBranch = userName + "/" + curentTopicBranch
|
||||||
} else {
|
|
||||||
headBranch = curentTopicBranch
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check if a AGit pull request already exist for this branch.
|
||||||
pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, repo.ID, headBranch, baseBranchName, issues_model.PullRequestFlowAGit)
|
pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, repo.ID, headBranch, baseBranchName, issues_model.PullRequestFlowAGit)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
if !issues_model.IsErrPullRequestNotExist(err) {
|
if !issues_model.IsErrPullRequestNotExist(err) {
|
||||||
return nil, fmt.Errorf("Failed to get unmerged agit flow pull request in repository: %s/%s Error: %w", ownerName, repoName, err)
|
return nil, fmt.Errorf("failed to get unmerged AGit flow pull request in repository %q: %w", repo.FullName(), err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// automatically fill out the title and the description from the first commit.
|
// Automatically fill out the title and the description from the first commit.
|
||||||
shouldGetCommit := len(title) == 0 || len(description) == 0
|
shouldGetCommit := len(title) == 0 || len(description) == 0
|
||||||
|
|
||||||
var commit *git.Commit
|
var commit *git.Commit
|
||||||
if shouldGetCommit {
|
if shouldGetCommit {
|
||||||
commit, err = gitRepo.GetCommit(opts.NewCommitIDs[i])
|
commit, err = gitRepo.GetCommit(opts.NewCommitIDs[i])
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("Failed to get commit %s in repository: %s/%s Error: %w", opts.NewCommitIDs[i], ownerName, repoName, err)
|
return nil, fmt.Errorf("failed to get commit %s in repository %q: %w", opts.NewCommitIDs[i], repo.FullName(), err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if !hasTitle || len(title) == 0 {
|
if !hasTitle || len(title) == 0 {
|
||||||
|
@ -122,11 +118,6 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
|
||||||
_, description, _ = strings.Cut(commit.CommitMessage, "\n\n")
|
_, description, _ = strings.Cut(commit.CommitMessage, "\n\n")
|
||||||
}
|
}
|
||||||
|
|
||||||
pusher, err := user_model.GetUserByID(ctx, opts.UserID)
|
|
||||||
if err != nil {
|
|
||||||
return nil, fmt.Errorf("Failed to get user. Error: %w", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
prIssue := &issues_model.Issue{
|
prIssue := &issues_model.Issue{
|
||||||
RepoID: repo.ID,
|
RepoID: repo.ID,
|
||||||
Title: title,
|
Title: title,
|
||||||
|
@ -150,12 +141,11 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := pull_service.NewPullRequest(ctx, repo, prIssue, []int64{}, []string{}, pr, []int64{}); err != nil {
|
if err := pull_service.NewPullRequest(ctx, repo, prIssue, []int64{}, []string{}, pr, []int64{}); err != nil {
|
||||||
return nil, err
|
return nil, fmt.Errorf("unable to create new pull request: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID)
|
log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID)
|
||||||
|
|
||||||
objectFormat, _ := gitRepo.GetObjectFormat()
|
|
||||||
results = append(results, private.HookProcReceiveRefResult{
|
results = append(results, private.HookProcReceiveRefResult{
|
||||||
Ref: pr.GetGitRefName(),
|
Ref: pr.GetGitRefName(),
|
||||||
OriginalRef: opts.RefFullNames[i],
|
OriginalRef: opts.RefFullNames[i],
|
||||||
|
@ -165,55 +155,57 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
// update exist pull request
|
// Update an existing pull request.
|
||||||
if err := pr.LoadBaseRepo(ctx); err != nil {
|
if err := pr.LoadBaseRepo(ctx); err != nil {
|
||||||
return nil, fmt.Errorf("Unable to load base repository for PR[%d] Error: %w", pr.ID, err)
|
return nil, fmt.Errorf("unable to load base repository for PR[%d]: %w", pr.ID, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
oldCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
|
oldCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("Unable to get ref commit id in base repository for PR[%d] Error: %w", pr.ID, err)
|
return nil, fmt.Errorf("unable to get commit id of reference[%s] in base repository for PR[%d]: %w", pr.GetGitRefName(), pr.ID, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Do not process this change if nothing was changed.
|
||||||
if oldCommitID == opts.NewCommitIDs[i] {
|
if oldCommitID == opts.NewCommitIDs[i] {
|
||||||
results = append(results, private.HookProcReceiveRefResult{
|
results = append(results, private.HookProcReceiveRefResult{
|
||||||
OriginalRef: opts.RefFullNames[i],
|
OriginalRef: opts.RefFullNames[i],
|
||||||
OldOID: opts.OldCommitIDs[i],
|
OldOID: opts.OldCommitIDs[i],
|
||||||
NewOID: opts.NewCommitIDs[i],
|
NewOID: opts.NewCommitIDs[i],
|
||||||
Err: "new commit is same with old commit",
|
Err: "The new commit is the same as the old commit",
|
||||||
})
|
})
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// If the force push option was not set, ensure that this change isn't a force push.
|
||||||
if !forcePush {
|
if !forcePush {
|
||||||
output, _, err := git.NewCommand(ctx, "rev-list", "--max-count=1").AddDynamicArguments(oldCommitID, "^"+opts.NewCommitIDs[i]).RunStdString(&git.RunOpts{Dir: repo.RepoPath(), Env: os.Environ()})
|
output, _, err := git.NewCommand(ctx, "rev-list", "--max-count=1").AddDynamicArguments(oldCommitID, "^"+opts.NewCommitIDs[i]).RunStdString(&git.RunOpts{Dir: repo.RepoPath(), Env: os.Environ()})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("Fail to detect force push: %w", err)
|
return nil, fmt.Errorf("failed to detect a force push: %w", err)
|
||||||
} else if len(output) > 0 {
|
} else if len(output) > 0 {
|
||||||
results = append(results, private.HookProcReceiveRefResult{
|
results = append(results, private.HookProcReceiveRefResult{
|
||||||
OriginalRef: opts.RefFullNames[i],
|
OriginalRef: opts.RefFullNames[i],
|
||||||
OldOID: opts.OldCommitIDs[i],
|
OldOID: opts.OldCommitIDs[i],
|
||||||
NewOID: opts.NewCommitIDs[i],
|
NewOID: opts.NewCommitIDs[i],
|
||||||
Err: "request `force-push` push option",
|
Err: "Updates were rejected because the tip of your current branch is behind its remote counterpart. If this is intentional, set the `force-push` option by adding `-o force-push=true` to your `git push` command.",
|
||||||
})
|
})
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Set the new commit as reference of the pull request.
|
||||||
pr.HeadCommitID = opts.NewCommitIDs[i]
|
pr.HeadCommitID = opts.NewCommitIDs[i]
|
||||||
if err = pull_service.UpdateRef(ctx, pr); err != nil {
|
if err = pull_service.UpdateRef(ctx, pr); err != nil {
|
||||||
return nil, fmt.Errorf("Failed to update pull ref. Error: %w", err)
|
return nil, fmt.Errorf("failed to update the reference of the pull request: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Add the pull request to the merge conflicting checker queue.
|
||||||
pull_service.AddToTaskQueue(ctx, pr)
|
pull_service.AddToTaskQueue(ctx, pr)
|
||||||
pusher, err := user_model.GetUserByID(ctx, opts.UserID)
|
|
||||||
if err != nil {
|
if err := pr.LoadIssue(ctx); err != nil {
|
||||||
return nil, fmt.Errorf("Failed to get user. Error: %w", err)
|
return nil, fmt.Errorf("failed to load the issue of the pull request: %w", err)
|
||||||
}
|
|
||||||
err = pr.LoadIssue(ctx)
|
|
||||||
if err != nil {
|
|
||||||
return nil, fmt.Errorf("Failed to load pull issue. Error: %w", err)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Create and notify about the new commits.
|
||||||
comment, err := pull_service.CreatePushPullComment(ctx, pusher, pr, oldCommitID, opts.NewCommitIDs[i])
|
comment, err := pull_service.CreatePushPullComment(ctx, pusher, pr, oldCommitID, opts.NewCommitIDs[i])
|
||||||
if err == nil && comment != nil {
|
if err == nil && comment != nil {
|
||||||
notify_service.PullRequestPushCommits(ctx, pusher, pr, comment)
|
notify_service.PullRequestPushCommits(ctx, pusher, pr, comment)
|
||||||
|
|
|
@ -936,6 +936,54 @@ func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headB
|
||||||
assert.Contains(t, pr.Issue.Content, "custom")
|
assert.Contains(t, pr.Issue.Content, "custom")
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
t.Run("Force push", func(t *testing.T) {
|
||||||
|
defer tests.PrintCurrentTest(t)()
|
||||||
|
|
||||||
|
upstreamGitRepo, err := git.OpenRepository(git.DefaultContext, filepath.Join(setting.RepoRootPath, ctx.Username, ctx.Reponame+".git"))
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer upstreamGitRepo.Close()
|
||||||
|
|
||||||
|
_, _, gitErr := git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments("HEAD:refs/for/master/" + headBranch + "-force-push").RunStdString(&git.RunOpts{Dir: dstPath})
|
||||||
|
require.NoError(t, gitErr)
|
||||||
|
|
||||||
|
unittest.AssertCount(t, &issues_model.PullRequest{}, pullNum+6)
|
||||||
|
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
|
||||||
|
HeadRepoID: repo.ID,
|
||||||
|
Flow: issues_model.PullRequestFlowAGit,
|
||||||
|
Index: pr1.Index + 5,
|
||||||
|
})
|
||||||
|
|
||||||
|
headCommitID, err := upstreamGitRepo.GetRefCommitID(pr.GetGitRefName())
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
_, _, gitErr = git.NewCommand(git.DefaultContext, "reset", "--hard", "HEAD~1").RunStdString(&git.RunOpts{Dir: dstPath})
|
||||||
|
require.NoError(t, gitErr)
|
||||||
|
|
||||||
|
t.Run("Fails", func(t *testing.T) {
|
||||||
|
defer tests.PrintCurrentTest(t)()
|
||||||
|
|
||||||
|
_, stdErr, gitErr := git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments("HEAD:refs/for/master/" + headBranch + "-force-push").RunStdString(&git.RunOpts{Dir: dstPath})
|
||||||
|
assert.Error(t, gitErr)
|
||||||
|
|
||||||
|
assert.Contains(t, stdErr, "-o force-push=true")
|
||||||
|
|
||||||
|
currentHeadCommitID, err := upstreamGitRepo.GetRefCommitID(pr.GetGitRefName())
|
||||||
|
assert.NoError(t, err)
|
||||||
|
assert.EqualValues(t, headCommitID, currentHeadCommitID)
|
||||||
|
})
|
||||||
|
t.Run("Succeeds", func(t *testing.T) {
|
||||||
|
defer tests.PrintCurrentTest(t)()
|
||||||
|
|
||||||
|
_, _, gitErr := git.NewCommand(git.DefaultContext, "push", "origin", "-o", "force-push=true").AddDynamicArguments("HEAD:refs/for/master/" + headBranch + "-force-push").RunStdString(&git.RunOpts{Dir: dstPath})
|
||||||
|
assert.NoError(t, gitErr)
|
||||||
|
|
||||||
|
currentHeadCommitID, err := upstreamGitRepo.GetRefCommitID(pr.GetGitRefName())
|
||||||
|
assert.NoError(t, err)
|
||||||
|
assert.NotEqualValues(t, headCommitID, currentHeadCommitID)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
t.Run("Merge", doAPIMergePullRequest(*ctx, ctx.Username, ctx.Reponame, pr1.Index))
|
t.Run("Merge", doAPIMergePullRequest(*ctx, ctx.Username, ctx.Reponame, pr1.Index))
|
||||||
t.Run("CheckoutMasterAgain", doGitCheckoutBranch(dstPath, "master"))
|
t.Run("CheckoutMasterAgain", doGitCheckoutBranch(dstPath, "master"))
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in a new issue