fix(worker): make modify/delete and rename/delete merge conflicts visible instead of silently staging them

ConflictMarkerParser only recognizes text-marker conflicts, so a modify/delete or
rename/delete conflict (git never writes markers for those) was invisible to the
resolver: GetConflictDocumentsAsync read it as an already-resolved stable file, and
ContinueMergeAsync's marker scan let it through, so `git add` on the unmerged path
silently kept whichever side wasn't deleted.

GetConflictDocumentsAsync now tells a modify/delete-style conflict (one index stage
missing) apart from an ordinary already-resolved file (both stages present) and
synthesizes a real whole-file conflict block from the index stages so it shows up in
the resolver. ContinueMergeAsync compares such a path against git's own default
checkout content before treating it as resolved, so an untouched file blocks the
merge instead of being silently staged. Resolving to the deleted side (empty content)
now runs `git rm` instead of staging a tracked, zero-byte file.
This commit is contained in:
Mika Kuns
2026-08-26 15:25:42 +02:00
parent c4928b4def
commit 533a287eb9
3 changed files with 176 additions and 13 deletions
+26
View File
@@ -376,6 +376,32 @@ public sealed class GitService
throw new InvalidOperationException($"git add '{path}' failed (exit {exitCode}): {stderr}"); throw new InvalidOperationException($"git add '{path}' failed (exit {exitCode}): {stderr}");
} }
/// <summary>
/// Stages <paramref name="path"/> as deleted (`git rm -f`), removing it from both the index
/// and, if present, the working tree. Used to resolve a merge conflict to "keep the
/// deletion" — unlike <see cref="AddPathAsync"/>, which would stage an empty file as a
/// tracked, zero-byte file rather than actually removing it.
/// </summary>
public async Task RemovePathAsync(string repoDir, string path, CancellationToken ct = default)
{
var (exitCode, _, stderr) = await RunGitAsync(repoDir, ["rm", "-f", "--", path], ct);
if (exitCode != 0)
throw new InvalidOperationException($"git rm '{path}' failed (exit {exitCode}): {stderr}");
}
/// <summary>
/// Content of <paramref name="path"/> at a merge stage (1=base, 2=ours, 3=theirs) via
/// `git show :&lt;stage&gt;:&lt;path&gt;`. Null when that stage doesn't exist for the path —
/// the conventional way to tell a modify/delete or rename/delete conflict apart from an
/// ordinary content conflict (git never writes `&lt;&lt;&lt;&lt;&lt;&lt;&lt;` markers for those,
/// it just leaves whichever side wasn't deleted in the working tree).
/// </summary>
public async Task<string?> ShowConflictStageAsync(string repoDir, int stage, string path, CancellationToken ct = default)
{
var (exitCode, stdout, _) = await RunGitAsync(repoDir, ["show", $":{stage}:{path}"], ct, trimOutput: false);
return exitCode == 0 ? stdout : null;
}
/// <summary> /// <summary>
/// Non-destructive mergeability probe via `git merge-tree --write-tree`. Writes only /// Non-destructive mergeability probe via `git merge-tree --write-tree`. Writes only
/// loose objects — the working tree, index, and refs are left untouched. /// loose objects — the working tree, index, and refs are left untouched.
@@ -520,20 +520,48 @@ public sealed class TaskMergeService
var oldTargetTip = await _git.RevParseHeadAsync(list.WorkingDir, ct); var oldTargetTip = await _git.RevParseHeadAsync(list.WorkingDir, ct);
// Validate BEFORE staging: `git add` marks a conflicted path resolved regardless of // Validate BEFORE staging: `git add` marks a conflicted path resolved regardless of
// its content, so an unresolved file with markers still in it would otherwise get // its content, so an unresolved file needs a positively-checked resolution, not just
// staged (and committed) as-is. Check text content for markers first; binary files // "whatever happens to be on disk". For an ordinary content conflict, git always
// can't carry markers, so they're left to the post-stage index check below. // starts the file with markers, so their absence reliably means someone resolved it
// (in-app or in an external editor). A modify/delete or rename/delete conflict never
// gets markers at all (GetConflictDocumentsAsync) — the same marker-less state also
// describes a file nobody has touched — so those are additionally checked against
// what git itself left on disk by default (whichever side wasn't deleted); still
// matching that default means still unresolved.
var unresolved = await _git.ListConflictedFilesAsync(list.WorkingDir, ct); var unresolved = await _git.ListConflictedFilesAsync(list.WorkingDir, ct);
var stillConflicted = new List<string>(); var stillConflicted = new List<string>();
var toAdd = new List<string>();
var toRemove = new List<string>();
foreach (var path in unresolved) foreach (var path in unresolved)
{ {
var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar)); var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar));
string text; string? text = null;
if (File.Exists(full))
{
try { text = await File.ReadAllTextAsync(full, ct); } try { text = await File.ReadAllTextAsync(full, ct); }
catch { continue; } catch { /* treated as unresolved below */ }
}
if (!LooksBinary(text) && ConflictMarkerParser.HasConflicts(text)) if (text is not null && (LooksBinary(text) || ConflictMarkerParser.HasConflicts(text)))
{
stillConflicted.Add(path); stillConflicted.Add(path);
continue;
}
var oursStage = await _git.ShowConflictStageAsync(list.WorkingDir, 2, path, ct);
var theirsStage = await _git.ShowConflictStageAsync(list.WorkingDir, 3, path, ct);
var isModifyDeleteStyle = oursStage is null || theirsStage is null;
if (isModifyDeleteStyle &&
NormalizeLineEndings(text ?? "") == NormalizeLineEndings(oursStage ?? theirsStage ?? ""))
{
stillConflicted.Add(path);
continue;
}
if (text is null || text.Length == 0)
toRemove.Add(path);
else
toAdd.Add(path);
} }
if (stillConflicted.Count > 0) if (stillConflicted.Count > 0)
@@ -550,8 +578,10 @@ public sealed class TaskMergeService
// Stage exactly the resolved conflict paths — never `git add -A`, which would sweep // Stage exactly the resolved conflict paths — never `git add -A`, which would sweep
// untracked/unrelated changes left by other sessions into this merge commit (the // untracked/unrelated changes left by other sessions into this merge commit (the
// target working dir is shared). // target working dir is shared).
foreach (var path in unresolved) foreach (var path in toAdd)
await _git.AddPathAsync(list.WorkingDir, path, ct); await _git.AddPathAsync(list.WorkingDir, path, ct);
foreach (var path in toRemove)
await _git.RemovePathAsync(list.WorkingDir, path, ct);
var remaining = await _git.ListConflictedFilesAsync(list.WorkingDir, ct); var remaining = await _git.ListConflictedFilesAsync(list.WorkingDir, ct);
if (remaining.Count > 0) if (remaining.Count > 0)
@@ -680,6 +710,15 @@ public sealed class TaskMergeService
/// <summary> /// <summary>
/// Reads each conflicted working-tree file and parses its conflict markers into line-level /// Reads each conflicted working-tree file and parses its conflict markers into line-level
/// segments (with the diff3 merge base when present). Binary files are flagged and skipped. /// segments (with the diff3 merge base when present). Binary files are flagged and skipped.
/// A path with no text markers on disk is either an ordinary content conflict that's already
/// been resolved (git keeps both index stages until the path is staged, whether or not the
/// working tree still looks conflicted — reads as a single stable segment, 0 hunks left), or
/// a modify/delete or rename/delete conflict — git never writes markers for those at all, it
/// just leaves whichever side wasn't deleted sitting in the working tree, so the same
/// "no markers" state also describes an untouched file. The index tells the two apart: a
/// modify/delete-style conflict only ever populates ONE of the ours/theirs stages. Those are
/// synthesized into a single whole-file conflict block straight from the stages (empty side
/// = that side deleted the path) so the resolver still shows a real choice.
/// </summary> /// </summary>
public async Task<ConflictDocuments> GetConflictDocumentsAsync(string taskId, CancellationToken ct) public async Task<ConflictDocuments> GetConflictDocumentsAsync(string taskId, CancellationToken ct)
{ {
@@ -692,21 +731,53 @@ public sealed class TaskMergeService
foreach (var path in files) foreach (var path in files)
{ {
var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar)); var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar));
string text; string? diskText = null;
try { text = await File.ReadAllTextAsync(full, ct); } if (File.Exists(full))
catch { text = ""; } {
try { diskText = await File.ReadAllTextAsync(full, ct); }
catch { /* unreadable — fall through to the index stages below */ }
}
if (LooksBinary(text)) if (diskText is not null && LooksBinary(diskText))
{ {
result.Add(new ConflictDocumentContent(path, true, Array.Empty<MergeSegment>())); result.Add(new ConflictDocumentContent(path, true, Array.Empty<MergeSegment>()));
continue; continue;
} }
result.Add(new ConflictDocumentContent(path, false, ConflictMarkerParser.Parse(text))); if (diskText is not null && ConflictMarkerParser.HasConflicts(diskText))
{
result.Add(new ConflictDocumentContent(path, false, ConflictMarkerParser.Parse(diskText)));
continue;
}
var ours = await _git.ShowConflictStageAsync(list.WorkingDir, 2, path, ct);
var theirs = await _git.ShowConflictStageAsync(list.WorkingDir, 3, path, ct);
if (ours is not null && theirs is not null)
{
// Ordinary two-sided conflict, already resolved (no markers left) but not yet staged.
result.Add(new ConflictDocumentContent(path, false,
new[] { MergeSegment.Stable(diskText ?? "") }));
continue;
}
if (LooksBinary(ours ?? "") || LooksBinary(theirs ?? ""))
{
result.Add(new ConflictDocumentContent(path, true, Array.Empty<MergeSegment>()));
continue;
}
var @base = await _git.ShowConflictStageAsync(list.WorkingDir, 1, path, ct);
result.Add(new ConflictDocumentContent(
path, false, new[] { MergeSegment.Conflict(ours ?? "", @base, theirs ?? "") }));
} }
return new ConflictDocuments(taskId, result); return new ConflictDocuments(taskId, result);
} }
// Working-tree checkouts can go through autocrlf while `git show :stage:path` never does —
// normalize before comparing the two or an untouched file reads as "resolved" on a machine
// with autocrlf enabled.
private static string NormalizeLineEndings(string text) => text.Replace("\r\n", "\n").Replace('\r', '\n');
// A NUL byte in the head of the file is the conventional binary sniff. // A NUL byte in the head of the file is the conventional binary sniff.
private static bool LooksBinary(string text) private static bool LooksBinary(string text)
{ {
@@ -722,6 +793,15 @@ public sealed class TaskMergeService
if (string.IsNullOrWhiteSpace(list.WorkingDir)) if (string.IsNullOrWhiteSpace(list.WorkingDir))
throw new InvalidOperationException("list has no working directory"); throw new InvalidOperationException("list has no working directory");
if (content.Length == 0)
{
// An empty resolution for a whole-file conflict (modify/delete, rename/delete) means
// "keep the deletion" — `git add` on an empty file would instead stage it as a
// tracked, zero-byte file, which is not what accepting the deleted side means.
await _git.RemovePathAsync(list.WorkingDir, path, ct);
return;
}
var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar)); var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar));
await File.WriteAllTextAsync(full, content, ct); await File.WriteAllTextAsync(full, content, ct);
await _git.AddPathAsync(list.WorkingDir, path, ct); await _git.AddPathAsync(list.WorkingDir, path, ct);
@@ -1109,6 +1109,63 @@ public class TaskMergeServiceTests : IDisposable
Assert.Contains(file.Segments, s => !s.IsConflict && s.Text.Contains("line1")); Assert.Contains(file.Segments, s => !s.IsConflict && s.Text.Contains("line1"));
} }
// git never writes <<<<<<< markers for a modify/delete conflict -- it just leaves the
// modifying side's content sitting in the working tree, which used to read as an
// already-resolved stable file (no conflict at all) to both GetConflictDocumentsAsync and
// ContinueMergeAsync's marker scan, so `continue_merge` would silently keep the modification.
[Fact]
public async Task ModifyDeleteConflict_IsSynthesizedAndCannotBeSilentlyStaged()
{
if (!GitRepoFixture.IsGitAvailable()) return;
var db = NewDb();
var repo = NewRepo();
GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main");
var wtPath = Path.Combine(Path.GetTempPath(), $"wt_{Guid.NewGuid():N}");
_wtCleanups.Add((repo.RepoDir, wtPath));
GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", "-b", "claudedo/modify-delete", wtPath, repo.BaseCommit);
File.WriteAllText(Path.Combine(wtPath, "README.md"), "# modified by branch\n");
GitRepoFixture.RunGit(wtPath, "commit", "-am", "branch modifies README");
GitRepoFixture.RunGit(repo.RepoDir, "rm", "README.md");
GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "main deletes README");
var (_, task) = await SeedListAndTask(db, workingDir: repo.RepoDir, status: TaskStatus.Done);
await SeedWorktree(db, task.Id, wtPath, "claudedo/modify-delete", repo.BaseCommit);
var (svc, _) = BuildService(db);
var merge = await svc.MergeAsync(task.Id, "main", removeWorktree: false, "msg",
leaveConflictsInTree: true, CancellationToken.None);
Assert.Equal(TaskMergeService.StatusConflict, merge.Status);
// No <<<<<<< markers anywhere -- git left the branch's modification as-is.
Assert.DoesNotContain("<<<<<<<", File.ReadAllText(Path.Combine(repo.RepoDir, "README.md")));
var docs = await svc.GetConflictDocumentsAsync(task.Id, CancellationToken.None);
var file = Assert.Single(docs.Files);
Assert.EndsWith("README.md", file.Path.Replace('\\', '/'));
Assert.False(file.IsBinary);
var conflict = Assert.Single(file.Segments.Where(s => s.IsConflict).ToList());
Assert.Equal("", conflict.Ours); // ours (main) deleted it
Assert.Contains("modified by branch", conflict.Theirs); // theirs (branch) modified it
// Nobody resolved it — continue must refuse rather than silently keeping the modification.
var headBefore = GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim();
var refused = await svc.ContinueMergeAsync(task.Id, CancellationToken.None);
Assert.Equal(TaskMergeService.StatusConflict, refused.Status);
Assert.Contains("README.md", refused.ConflictFiles);
Assert.Equal(headBefore, GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim());
Assert.True(await new GitService().IsMidMergeAsync(repo.RepoDir));
// Explicitly resolving to the deleted (ours) side must `git rm`, not stage an empty file.
await svc.WriteResolutionAsync(task.Id, "README.md", "", CancellationToken.None);
var result = await svc.ContinueMergeAsync(task.Id, CancellationToken.None);
Assert.Equal(TaskMergeService.StatusMerged, result.Status);
Assert.False(File.Exists(Path.Combine(repo.RepoDir, "README.md")));
Assert.False(await new GitService().IsMidMergeAsync(repo.RepoDir));
}
[Fact] [Fact]
public async Task ApproveAndMergeAsync_NoWorktree_MarksDone() public async Task ApproveAndMergeAsync_NoWorktree_MarksDone()
{ {