Add UI-based merge conflict resolution#166
Conversation
…t conflict resolution
|
/crush_fast {{openrouter, google/gemini-3.1-pro-preview, google/gemini-3-flash-preview}} AI review started. |
Advanced AI Review
Click to expand review1. 🔴 Conflict wrappers are appended to the bottom of the table, breaking file order
In To fix this, insert the
const firstRow = group[0];
const parentNode = firstRow.parentNode;
// Create wrapper container row
const wrapperRow = document.createElement('tr');
wrapperRow.className = 'conflict-wrapper-row';
// Insert wrapper into the table tree BEFORE we detach the conflict lines
if (parentNode) {
parentNode.insertBefore(wrapperRow, firstRow);
}
const wrapperCell = document.createElement('td');
wrapperCell.colSpan = 6;Be sure to remove the faulty insert statement at the end of the block: - // Insert wrapper before the first conflict line's original position
- parentNode.insertBefore(wrapperRow, null);2. 🔴 Unmatched additions and deletions are hidden as context lines instead of conflicts
In Because your JS gathers conflicts based on Separate actual identical context lines (
3. 🟡 Active state styling is missing on the Conflicts tab
The PR description states "add active state styling to conflicts tab in change request menu". However, the anchor tag for the new Conflicts tab lacks the conditional
4. 🟡 Incorrect and mismatching backgrounds for right-side code
In It is better to rely on
/* Right code content: match gutter #FFE7D1 */
.conflict-inner-table .add-code .lines-code-new {
- background: #ffe7d1 !important;
+ background: #e5e9ff !important;
}5. ⚪️
|
…licts via POST endpoint
…T helper and replace encoding/json with internal module
… in pull request logic
…pport for multiple parent commits and object alternates
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…on via editor integration
… errors When submission failed, the button text was permanently changed to the error message. Now original text is saved and restored on failure, and errors are shown via showErrorToast so the button remains clearly actionable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…andlers Without this, if a user selects an ignore-whitespace mode via query param, the rendered diff groups differ from what extractConflictGroups computes at submit time, causing a group count mismatch and a rejected submission. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sions The disabled squash button on conflicted PRs was shown unconditionally, including to users with no merge rights. Gate it behind the same AllowMerge + IsBaseRepoOwner + AllowSquash checks used for the real merge form. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nt races Two related issues: 1. If the base branch advances between page load and submission, applyConflictTexts would splice resolutions at wrong line positions. Fix: include base commit ID in the POST body; server rejects with 409 if the branch has since moved. 2. No lock was held, so a concurrent merge/update on the same PR could race. Fix: acquire the PR working lock (same key used by pull_service.Merge and Update) before reading commit IDs and writing the resolution commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/crush_fast {{openrouter, openai/gpt-5.5}} AI review started. |
Advanced AI Review
Click to expand review1. 🔴 Conflict resolution creates a fake merge commit that can revert base-branch changes
if err := t.SetDefaultIndex(ctx); err != nil {
ctx.ServerError("SetDefaultIndex", err)
return
}
for _, rf := range resolvedFiles {
blobHash, err := t.HashObjectAndWrite(ctx, bytes.NewReader(rf.content))
// ...
if err := t.AddObjectToIndex(ctx, "100644", blobHash, rf.path); err != nil {
// ...
}
}
treeHash, err := t.WriteTree(ctx)
// ...
mergeCommitID, err := t.CommitTree(ctx, &files_service.CommitTreeUserOptions{
ParentCommitID: headCommitID,
AdditionalParentCommitIDs: []string{baseCommitID},
TreeHash: treeHash,
CommitMessage: "Resolve merge conflicts",
DoerUser: ctx.Doer,
})This starts from the head branch index, overwrites only conflicted files, then records Suggested fix: perform a real merge in the temp repository, keep Git’s auto-merged index entries, then replace only unresolved paths with user resolutions. // services/repository/files/temp_repo.go
func (t *TemporaryUploadRepository) MergeNoCommitAllowConflicts(ctx context.Context, commitID string) error {
stdout := new(bytes.Buffer)
stderr := new(bytes.Buffer)
err := gitcmd.NewCommand("merge", "--no-ff", "--no-commit").AddDynamicArguments(commitID).
Run(ctx, &gitcmd.RunOpts{
Dir: t.basePath,
Stdout: stdout,
Stderr: stderr,
})
if err == nil {
return nil
}
unmergedStdout, _, unmergedErr := gitcmd.NewCommand("diff", "--name-only", "--diff-filter=U").
RunStdString(ctx, &gitcmd.RunOpts{Dir: t.basePath})
if unmergedErr != nil {
return fmt.Errorf("merge failed and unmerged paths could not be inspected: %w\nstdout: %s\nstderr: %s", err, stdout.String(), stderr.String())
}
if strings.TrimSpace(unmergedStdout) == "" {
return fmt.Errorf("merge failed without conflicts: %w\nstdout: %s\nstderr: %s", err, stdout.String(), stderr.String())
}
return nil
}Then in if err := t.Clone(ctx, pull.HeadBranch, false); err != nil {
ctx.ServerError("Clone", err)
return
}
if headRepo.ID != pull.BaseRepoID {
if err := t.AddObjectAlternates(ctx.Repo.Repository.RepoPath()); err != nil {
ctx.ServerError("AddObjectAlternates", err)
return
}
}
if err := t.MergeNoCommitAllowConflicts(ctx, baseCommitID); err != nil {
ctx.ServerError("MergeNoCommitAllowConflicts", err)
return
}
for _, rf := range resolvedFiles {
blobHash, err := t.HashObjectAndWrite(ctx, bytes.NewReader(rf.content))
if err != nil {
ctx.ServerError("HashObjectAndWrite", err)
return
}
if err := t.AddObjectToIndex(ctx, rf.mode, blobHash, rf.path); err != nil {
ctx.ServerError("AddObjectToIndex", err)
return
}
}
treeHash, err := t.WriteTree(ctx)2. 🔴 The UI/backend treat every normal diff hunk in conflicted files as a conflict
diff, err := gitdiff.GetDiffForRender(ctx, ctx.Repo.RepoLink, gitRepo, &gitdiff.DiffOptions{
BeforeCommitID: beforeCommitID,
AfterCommitID: headCommitID,
SkipTo: ctx.FormString("skip-to"),
MaxLines: maxLines,
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: maxFiles,
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(whitespaceBehavior),
}, files...)
diff, err := gitdiff.GetDiffForRender(ctx, ctx.Repo.RepoLink, gitRepo, &gitdiff.DiffOptions{
BeforeCommitID: baseCommitID,
AfterCommitID: headCommitID,
MaxLines: -1,
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: 1,
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(submitWhitespaceBehavior),
}, fileReq.Path)A Suggested fix: derive conflict groups from the unmerged index after an actual merge attempt, not from type conflictStageFile struct {
Path string
BaseBlobID string // stage 1
OursBlobID string // stage 2: PR head
TheirsBlobID string // stage 3: base branch
}
func (t *TemporaryUploadRepository) ListUnmergedFiles(ctx context.Context) ([]conflictStageFile, error) {
stdout, _, err := gitcmd.NewCommand("ls-files", "-u", "-z").
RunStdString(ctx, &gitcmd.RunOpts{Dir: t.basePath})
if err != nil {
return nil, err
}
entries := strings.Split(strings.TrimRight(stdout, "\x00"), "\x00")
byPath := map[string]*conflictStageFile{}
for _, entry := range entries {
fields := strings.Fields(entry)
if len(fields) < 4 {
continue
}
stageAndPath := fields[2]
stage, path, ok := strings.Cut(stageAndPath, "\t")
if !ok {
continue
}
item := byPath[path]
if item == nil {
item = &conflictStageFile{Path: path}
byPath[path] = item
}
switch stage {
case "1":
item.BaseBlobID = fields[1]
case "2":
item.OursBlobID = fields[1]
case "3":
item.TheirsBlobID = fields[1]
}
}
files := make([]conflictStageFile, 0, len(byPath))
for _, item := range byPath {
files = append(files, *item)
}
slices.SortFunc(files, func(a, b conflictStageFile) int {
return strings.Compare(a.Path, b.Path)
})
return files, nil
}Use these unmerged files to render the conflict page and to validate submitted conflict indices. 3. 🔴 Authorization bypass: PR posters can push conflict-resolution commits without confirmed head-branch write permission
allowedUpdateByMerge, _, err := pull_service.IsUserAllowedToUpdate(ctx, pull, ctx.Doer)
if err != nil {
ctx.ServerError("IsUserAllowedToUpdate", err)
return
}
isPosterOrAdmin := ctx.Doer.ID == issue.PosterID || ctx.Doer.IsAdmin
if !isPosterOrAdmin && !allowedUpdateByMerge {
ctx.PlainText(http.StatusForbidden, "not allowed to resolve conflicts")
return
}
if err := t.PushWithOptions(ctx, ctx.Doer, mergeCommitID, pull.HeadBranch, true); err != nil {The backend explicitly lets the poster bypass Suggested fix: require current write/update permission to the head branch, and avoid internal push unless a separate server-side policy check has explicitly authorized bypassing hooks/protection. if err := pull.LoadHeadRepo(ctx); err != nil {
ctx.ServerError("LoadHeadRepo", err)
return
}
if pull.HeadRepo == nil {
ctx.PlainText(http.StatusBadRequest, "head repository not found")
return
}
headPerm, err := access_model.GetUserRepoPermission(ctx, pull.HeadRepo, ctx.Doer)
if err != nil {
ctx.ServerError("GetUserRepoPermission(head)", err)
return
}
if !headPerm.CanWrite(unit.TypeCode) && !ctx.Doer.IsAdmin {
ctx.PlainText(http.StatusForbidden, "not allowed to push to the change request branch")
return
}
allowedUpdateByMerge, _, err := pull_service.IsUserAllowedToUpdate(ctx, pull, ctx.Doer)
if err != nil {
ctx.ServerError("IsUserAllowedToUpdate", err)
return
}
if !allowedUpdateByMerge && !ctx.Doer.IsAdmin {
ctx.PlainText(http.StatusForbidden, "not allowed to resolve conflicts")
return
}And push through normal user hooks/protection: if err := t.PushWithOptions(ctx, ctx.Doer, mergeCommitID, pull.HeadBranch, false); err != nil {
if git.IsErrPushOutOfDate(err) {
ctx.PlainText(http.StatusConflict, "head branch was updated concurrently, please reload and try again")
return
}
ctx.ServerError("PushWithOptions", err)
return
}4. 🟡 Stale submissions only check the base SHA, not the head SHA
const issueLink = window.location.pathname.replace(/\/conflicts$/, '');
const baseCommitID = document.querySelector('#diff-file-boxes')?.getAttribute('data-base-commit-id') ?? '';
setButtonsState(true, 'Submitting…');
try {
const resp = await POST(window.location.pathname, {data: {baseCommitID, files}});
if req.BaseCommitID != "" && req.BaseCommitID != baseCommitID {
ctx.PlainText(http.StatusConflict, "base branch has been updated since you loaded this page, please reload and try again")
return
}If the PR head branch changes after the conflict page loads, the submitted conflict indices and editor text are applied to a different diff. The final push only catches changes that happen after the temp clone, not changes that happened between page render and submission. Suggested fix: include const diffBoxes = document.querySelector('#diff-file-boxes');
const baseCommitID = diffBoxes?.getAttribute('data-base-commit-id') ?? '';
const headCommitID = diffBoxes?.getAttribute('data-head-commit-id') ?? '';
const resp = await POST(window.location.pathname, {
data: {baseCommitID, headCommitID, files},
});type conflictResolutionRequest struct {
BaseCommitID string `json:"baseCommitID"`
HeadCommitID string `json:"headCommitID"`
Files []struct {
Path string `json:"path"`
Conflicts []struct {
Index int `json:"index"`
Text string `json:"text"`
} `json:"conflicts"`
} `json:"files"`
}
if req.BaseCommitID == "" || req.HeadCommitID == "" {
ctx.PlainText(http.StatusBadRequest, "missing base or head commit id")
return
}
if req.BaseCommitID != baseCommitID {
ctx.PlainText(http.StatusConflict, "base branch has been updated since you loaded this page, please reload and try again")
return
}
if req.HeadCommitID != headCommitID {
ctx.PlainText(http.StatusConflict, "change request branch has been updated since you loaded this page, please reload and try again")
return
}5. 🟡 Deleted/renamed/non-regular conflicted files either fail or lose metadata
headBlob, err := headCommit.GetBlobByPath(fileReq.Path)
if err != nil {
ctx.ServerError("GetBlobByPath(head)", err)
return
}
headReader, err := headBlob.DataAsync()
if err != nil {
ctx.ServerError("headBlob.DataAsync", err)
return
}
headContent, err := io.ReadAll(headReader)
if err := t.AddObjectToIndex(ctx, "100644", blobHash, rf.path); err != nil {A modify/delete conflict where the head side deleted the file cannot be resolved because Suggested fix: preserve the existing mode for regular files, allow missing head content for deleted files, and explicitly reject unsupported modes until they are implemented. type resolvedFile struct {
path string
mode string
content []byte
}
func readHeadFileForConflict(ctx *context.Context, commit *git.Commit, path string) ([]byte, string, error) {
entry, err := commit.GetTreeEntryByPath(path)
if err != nil {
if git.IsErrNotExist(err) {
return nil, git.EntryModeBlob.String(), nil
}
return nil, "", err
}
mode := entry.Mode()
if mode != git.EntryModeBlob && mode != git.EntryModeExec {
return nil, "", fmt.Errorf("unsupported conflicted file mode %s for %s", mode.String(), path)
}
blob := entry.Blob()
if blob.Size() > setting.UI.MaxDisplayFileSize {
return nil, "", fmt.Errorf("conflicted file %s is too large to resolve in the browser", path)
}
content, err := blob.GetBlobContent(setting.UI.MaxDisplayFileSize)
if err != nil {
return nil, "", err
}
return []byte(content), mode.String(), nil
}Use it during resolution: headContent, mode, err := readHeadFileForConflict(ctx, headCommit, fileReq.Path)
if err != nil {
ctx.PlainText(http.StatusBadRequest, err.Error())
return
}
resolvedFiles = append(resolvedFiles, resolvedFile{
path: fileReq.Path,
mode: mode,
content: resolved,
})And preserve the mode: if err := t.AddObjectToIndex(ctx, rf.mode, blobHash, rf.path); err != nil {
ctx.ServerError("AddObjectToIndex", err)
return
}6. 🟡 Binary/suppressed conflicts can render with no resolvable conflict controls
if (allConflictWrappers.length === 0) return;
// Number conflicts globally and add headers
numberConflicts(allConflictWrappers);
// Setup fold/unfold toggle
initFoldToggle();
// Setup submit button tracking
initSubmitTracking();If any conflicted file is binary or suppressed, no wrapper is created for that file. The submit buttons remain disabled forever, but the backend still requires every conflicted file to be present. Suggested fix: fail fast with an actionable message instead of rendering an unusable resolver, or implement a binary keep/use resolver. for _, file := range diff.Files {
if file.IsBin || file.IsIncomplete {
ctx.Data["DiffNotAvailable"] = true
ctx.Data["ConflictResolutionUnavailableReason"] = ctx.Tr("repo.pulls.conflicts_unsupported_file", file.Name)
ctx.HTML(http.StatusOK, tplPullConflicts)
return
}
}Template: Add locale: pulls.conflicts_unsupported_file = Conflict resolution is not available for "%s" because the file is binary or too large to display.Review generated using |
The base-branch staleness check (added previously) did not cover the case where the head branch is updated between page load and submission. If the head branch moves, the conflict diff indices no longer match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ution Two related issues: 1. The index entry was always written as "100644", so executable files (100755) lost their execute bit after conflict resolution. 2. A modify/delete conflict (head side deleted the file) caused a crash at GetBlobByPath. Now the missing head file is treated as empty content so the user's resolution (e.g. choosing the base version) can still be applied. Non-regular modes (symlinks, submodules) are rejected with a 400. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously, binary or suppressed conflicted files rendered no conflict wrappers, leaving the submit button permanently disabled with no explanation. Now the Go handler detects IsBin/IsIncomplete early and sets a template variable. The template shows a clear warning message and hides the fold/submit controls so the page is not misleadingly interactive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* MergeNoCommitAllowConflicts: runs git merge --no-ff --no-commit, treats MERGE_HEAD + unmerged entries as expected conflict state * listUnmergedIndexEntries: parses ls-files -u -z into structured entries * resolveIndexPath: clears all unmerged stages via mode=0 --index-info, installs resolved blob at stage 0, verifies post-condition * submitConflictResolution: clones head, merges base, resolves against actual unmerged index, commits with both parents * adds TestSubmitConflictResolutionPreservesBaseOnlyChanges regression test * release conflict lock before sync recheck
* use temp-repo merge in ViewPullConflicts to derive true unmerged files * treat stored ConflictedFiles as advisory in submit prevalidation * cover stale false-positive conflict lists in the integration test * guard temporary repo cleanup when clone/open fails
* reject symlink, submodule, and other non-regular unmerged stages * require locked base/head commit ID validation for conflict submissions * add unit and integration coverage
* returns the no-conflicts error after auth and PR state gates. * preserves authorization behavior for anonymous, forbidden, poster, and admin cases.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
web_src/js/features/repo-conflict-review.ts:605
- The submit flow uses hard-coded UI/toast strings (e.g. “Submitting…”, “Submit failed: …”, “network error”). These should be localized (via
window.config.i18nor template-provided strings) to keep the conflicts flow consistent with the rest of the app’s i18n behavior.
setButtonsState(true, 'Submitting…');
try {
const resp = await POST(window.location.pathname, {data: {baseCommitID, headCommitID, files}});
if (resp.ok) {
window.location.href = issueLink;
} else {
let msg: string;
try {
msg = await resp.text();
} catch {
msg = `HTTP ${resp.status}`;
}
setButtonsState(false);
showErrorToast(`Submit failed: ${msg}`);
}
} catch {
setButtonsState(false);
showErrorToast('Submit failed: network error');
}
Closes #153