Strict push/updateref with old commit id#36700
Conversation
GiteaBot
added
the
lgtm/need 2
There was a problem hiding this comment.
Pull request overview
This PR aims to make branch/ref updates safer by adding “old commit ID” protection (similar to --force-with-lease / conditional update-ref) to reduce the risk of overwriting concurrent changes during PR updates/merges and AGit ref updates.
Changes:
- Switch PR merge and rebase-update pushes to use
git.PushwithForceWithLease. - Add
gitrepo.UpdateRefWithOldand plumb an optionaloldCommitIDthroughpull_service.UpdateRefWithOldCommit. - Add tests covering
UpdateRefWithOldandPushOptions.ForceWithLease.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/pull/update_rebase.go | Uses --force-with-lease when pushing rebased head branch updates. |
| services/pull/pull.go | Adds UpdateRefWithOldCommit and uses conditional ref update in gitrepo. |
| services/pull/merge.go | Uses --force-with-lease for merge push to base branch. |
| services/agit/agit.go | Uses UpdateRefWithOldCommit to protect AGit pull ref updates. |
| modules/gitrepo/ref.go | Adds UpdateRefWithOld wrapper around git update-ref old-value argument. |
| modules/gitrepo/ref_test.go | Adds unit test for UpdateRefWithOld. |
| modules/git/repo_push_test.go | Adds unit test for PushOptions.ForceWithLease. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| oldCommitID, err := baseGitRepo.GetRefCommitID(git.BranchPrefix + pr.BaseBranch) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get base branch commit: %w", err) |
There was a problem hiding this comment.
The oldCommitID used for --force-with-lease is read from the base repo immediately before the push. That defeats the protection: if pr.BaseBranch advanced after the temp repo was fetched/merged, this lease will still match and the push can overwrite newer commits. The expected commit should be the base branch commit the merge was built on (e.g. the commit of original_base / mergeBaseSHA), so the push fails when the branch has moved during the merge.
| oldCommitID, err := baseGitRepo.GetRefCommitID(git.BranchPrefix + pr.BaseBranch) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to get base branch commit: %w", err) | |
| // Use the base commit the merge was built on (merge base) for the lease, so that | |
| // the push fails if the base branch has advanced since the merge was computed. | |
| oldCommitID := pr.MergeBase | |
| if oldCommitID == "" { | |
| // Fallback: preserve previous behavior if no merge base is recorded. | |
| oldCommitID, err = baseGitRepo.GetRefCommitID(git.BranchPrefix + pr.BaseBranch) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to get base branch commit: %w", err) | |
| } |
| @@ -64,43 +76,21 @@ func updateHeadByRebaseOnToBase(ctx context.Context, pr *issues_model.PullReques | |||
| headUser = pr.HeadRepo.Owner | |||
| } | |||
|
|
|||
| pushCmd := gitcmd.NewCommand("push", "-f", "head_repo"). | |||
| AddDynamicArguments(tmpRepoStagingBranch + ":" + git.BranchPrefix + pr.HeadBranch) | |||
|
|
|||
| // Push back to the head repository. | |||
| // TODO: this cause an api call to "/api/internal/hook/post-receive/...", | |||
| // that prevents us from doint the whole merge in one db transaction | |||
| mergeCtx.outbuf.Reset() | |||
|
|
|||
| if err := pushCmd. | |||
| WithEnv(repo_module.FullPushingEnvironment( | |||
| return git.Push(ctx, mergeCtx.tmpBasePath, git.PushOptions{ | |||
| Remote: "head_repo", | |||
| LocalRefName: tmpRepoStagingBranch, | |||
| Branch: git.BranchPrefix + pr.HeadBranch, | |||
| ForceWithLease: fmt.Sprintf("%s:%s", git.BranchPrefix+pr.HeadBranch, oldCommitID), | |||
| Env: repo_module.FullPushingEnvironment( | |||
There was a problem hiding this comment.
The lease commit is retrieved from pr.HeadRepo right before pushing, which only protects against changes in the narrow window between this read and the push. If the head branch moved after the temporary repo fetched tracking (i.e. during the rebase), this will still succeed and can overwrite someone else’s updates. Use the head/tracking commit ID that the rebase was based on (the fetched tracking ref) as the --force-with-lease expected value so the push fails if the branch changed during the operation.
| baseRepoPath := "../git/tests/repos/repo1_bare" | ||
| tempDir := t.TempDir() | ||
|
|
||
| require.NoError(t, git.Clone(ctx, baseRepoPath, tempDir, git.CloneRepoOptions{Bare: true})) | ||
|
|
There was a problem hiding this comment.
This test clones into tempDir := t.TempDir(), but git clone fails if the destination directory already exists (even if empty). Use a non-existent subdirectory (e.g. filepath.Join(t.TempDir(), "repo.git")) or remove the directory before cloning, otherwise the test will be flaky/fail consistently.
|
Does this have a dependency or relation to #36653 or can it merge independently? |
3 participants
To prevent accidentally updating references or force-pushing over someone else’s work, include the previous commit ID as a lease (e.g., use --force-with-lease).
Generated by a coding agent with Codex 5.2 LLM.