Skip to content

Commit db4113e

Browse files
lunnylunny
committed
Add transaction for auto merge
1 parent ec2f717 commit db4113e

File tree

3 files changed

+68
-36
lines changed

3 files changed

+68
-36
lines changed

‎services/automerge/automerge.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) {
287287

288288
if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil {
289289
log.Error("pull_service.Merge: %v", err)
290+
// FIXME: if merge failed, we should display some error message to the pull request page.
290291
return
291292
}
292293
}

‎services/pull/merge.go

Lines changed: 59 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -162,12 +162,6 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
162162
pullWorkingPool.CheckIn(fmt.Sprint(pr.ID))
163163
defer pullWorkingPool.CheckOut(fmt.Sprint(pr.ID))
164164

165-
// Removing an auto merge pull and ignore if not exist
166-
// FIXME: is this the correct point to do this? Shouldn't this be after IsMergeStyleAllowed?
167-
if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) {
168-
return err
169-
}
170-
171165
prUnit, err := pr.BaseRepo.GetUnit(ctx, unit.TypePullRequests)
172166
if err != nil {
173167
log.Error("pr.BaseRepo.GetUnit(unit.TypePullRequests): %v", err)
@@ -184,17 +178,31 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
184178
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
185179
}()
186180

187-
pr.MergedCommitID, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message)
181+
mergeCtx, cancel, err := doMerge(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message)
188182
if err != nil {
189183
return err
190184
}
185+
defer cancel()
191186

192-
pr.MergedUnix = timeutil.TimeStampNow()
193-
pr.Merger = doer
194-
pr.MergerID = doer.ID
187+
if err := db.WithTx(ctx, func(ctx context.Context) error {
188+
// Removing an auto merge pull and ignore if not exist
189+
if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) {
190+
return err
191+
}
192+
193+
pr.MergedCommitID = mergeCtx.mergeCommitID
194+
pr.MergedUnix = timeutil.TimeStampNow()
195+
pr.Merger = doer
196+
pr.MergerID = doer.ID
197+
if _, err := pr.SetMerged(ctx); err != nil {
198+
log.Error("SetMerged %-v: %v", pr, err)
199+
return err
200+
}
195201

196-
if _, err := pr.SetMerged(ctx); err != nil {
197-
log.Error("SetMerged %-v: %v", pr, err)
202+
_, err := doPush(ctx, mergeCtx, pr, doer)
203+
return err
204+
}); err != nil {
205+
return err
198206
}
199207

200208
if err := pr.LoadIssue(ctx); err != nil {
@@ -244,62 +252,82 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
244252
return nil
245253
}
246254

247-
// doMergeAndPush performs the merge operation without changing any pull information in database and pushes it up to the base repository
248-
func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (string, error) {
255+
func doMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (*mergeContext, context.CancelFunc, error) {
249256
// Clone base repo.
250257
mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID)
251258
if err != nil {
252-
return "", err
259+
return nil, nil, err
253260
}
254-
defer cancel()
255-
256261
// Merge commits.
257262
switch mergeStyle {
258263
case repo_model.MergeStyleMerge:
259264
if err := doMergeStyleMerge(mergeCtx, message); err != nil {
260-
return "", err
265+
cancel()
266+
return nil, nil, err
261267
}
262268
case repo_model.MergeStyleRebase, repo_model.MergeStyleRebaseMerge:
263269
if err := doMergeStyleRebase(mergeCtx, mergeStyle, message); err != nil {
264-
return "", err
270+
cancel()
271+
return nil, nil, err
265272
}
266273
case repo_model.MergeStyleSquash:
267274
if err := doMergeStyleSquash(mergeCtx, message); err != nil {
268-
return "", err
275+
cancel()
276+
return nil, nil, err
269277
}
270278
case repo_model.MergeStyleFastForwardOnly:
271279
if err := doMergeStyleFastForwardOnly(mergeCtx); err != nil {
272-
return "", err
280+
cancel()
281+
return nil, nil, err
273282
}
274283
default:
275-
return "", models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle}
284+
cancel()
285+
return nil, nil, models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle}
276286
}
277287

278288
// OK we should cache our current head and origin/headbranch
279-
mergeHeadSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "HEAD")
289+
mergeCtx.mergeHeadSHA, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "HEAD")
280290
if err != nil {
281-
return "", fmt.Errorf("Failed to get full commit id for HEAD: %w", err)
291+
cancel()
292+
return nil, nil, fmt.Errorf("Failed to get full commit id for HEAD: %w", err)
282293
}
283-
mergeBaseSHA, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "original_"+baseBranch)
294+
mergeCtx.mergeBaseSHA, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, "original_"+baseBranch)
284295
if err != nil {
285-
return "", fmt.Errorf("Failed to get full commit id for origin/%s: %w", pr.BaseBranch, err)
296+
cancel()
297+
return nil, nil, fmt.Errorf("Failed to get full commit id for origin/%s: %w", pr.BaseBranch, err)
286298
}
287-
mergeCommitID, err := git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, baseBranch)
299+
mergeCtx.mergeCommitID, err = git.GetFullCommitID(ctx, mergeCtx.tmpBasePath, baseBranch)
288300
if err != nil {
289-
return "", fmt.Errorf("Failed to get full commit id for the new merge: %w", err)
301+
cancel()
302+
return nil, nil, fmt.Errorf("Failed to get full commit id for the new merge: %w", err)
290303
}
291304

305+
return mergeCtx, cancel, nil
306+
}
307+
308+
// doMergeAndPush performs the merge operation without changing any pull information in database and pushes it up to the base repository
309+
func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (string, error) {
310+
mergeCtx, cancel, err := doMerge(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message)
311+
if err != nil {
312+
return "", err
313+
}
314+
defer cancel()
315+
316+
return doPush(ctx, mergeCtx, pr, doer)
317+
}
318+
319+
func doPush(ctx context.Context, mergeCtx *mergeContext, pr *issues_model.PullRequest, doer *user_model.User) (string, error) {
292320
// Now it's questionable about where this should go - either after or before the push
293321
// I think in the interests of data safety - failures to push to the lfs should prevent
294322
// the merge as you can always remerge.
295323
if setting.LFS.StartServer {
296-
if err := LFSPush(ctx, mergeCtx.tmpBasePath, mergeHeadSHA, mergeBaseSHA, pr); err != nil {
324+
if err := LFSPush(ctx, mergeCtx.tmpBasePath, mergeCtx.mergeHeadSHA, mergeCtx.mergeBaseSHA, pr); err != nil {
297325
return "", err
298326
}
299327
}
300328

301329
var headUser *user_model.User
302-
err = pr.HeadRepo.LoadOwner(ctx)
330+
err := pr.HeadRepo.LoadOwner(ctx)
303331
if err != nil {
304332
if !user_model.IsErrUserNotExist(err) {
305333
log.Error("Can't find user: %d for head repository in %-v: %v", pr.HeadRepo.OwnerID, pr, err)
@@ -344,7 +372,7 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
344372
mergeCtx.outbuf.Reset()
345373
mergeCtx.errbuf.Reset()
346374

347-
return mergeCommitID, nil
375+
return mergeCtx.mergeCommitID, nil
348376
}
349377

350378
func commitAndSignNoAuthor(ctx *mergeContext, message string) error {

‎services/pull/merge_prepare.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,14 @@ import (
2525

2626
type mergeContext struct {
2727
*prContext
28-
doer *user_model.User
29-
sig *git.Signature
30-
committer *git.Signature
31-
signKeyID string // empty for no-sign, non-empty to sign
32-
env []string
28+
doer *user_model.User
29+
sig *git.Signature
30+
committer *git.Signature
31+
signKeyID string // empty for no-sign, non-empty to sign
32+
env []string
33+
mergeHeadSHA string
34+
mergeBaseSHA string
35+
mergeCommitID string
3336
}
3437

3538
func (ctx *mergeContext) RunOpts() *git.RunOpts {

0 commit comments

Comments
 (0)