refactor #1
Binary file not shown.
@@ -46,7 +46,7 @@ def run_git_diff(repo: str, base: str, head: str) -> str:
|
||||
base_candidates = _build_ref_candidates(base)
|
||||
head_candidates = _build_ref_candidates(head)
|
||||
|
||||
stdout, stderr = _diff_with_candidates(repo, base_candidates, head_candidates)
|
||||
stdout, stderr, fallback_pairs = _diff_with_candidates(repo, base_candidates, head_candidates)
|
||||
if stdout is not None:
|
||||
return stdout
|
||||
|
||||
@@ -60,12 +60,20 @@ def run_git_diff(repo: str, base: str, head: str) -> str:
|
||||
# Ignore fetch errors; we'll raise the original error below.
|
||||
pass
|
||||
|
||||
stdout, stderr = _diff_with_candidates(repo, base_candidates, head_candidates)
|
||||
stdout, stderr, fallback_pairs_after_fetch = _diff_with_candidates(
|
||||
repo, base_candidates, head_candidates
|
||||
)
|
||||
if stdout is not None:
|
||||
return stdout
|
||||
|
||||
stdout, fallback_error = _fallback_diff_without_merge_base(
|
||||
repo, fallback_pairs + fallback_pairs_after_fetch
|
||||
)
|
||||
if stdout is not None:
|
||||
return stdout
|
||||
|
||||
# If still failing, raise a helpful error including stderr from git
|
||||
raise RuntimeError(stderr or first_error or "git diff failed")
|
||||
raise RuntimeError(fallback_error or stderr or first_error or "git diff failed")
|
||||
|
||||
|
||||
def _build_ref_candidates(ref: str) -> list[str]:
|
||||
@@ -75,26 +83,24 @@ def _build_ref_candidates(ref: str) -> list[str]:
|
||||
return candidates
|
||||
|
||||
|
||||
def _diff_with_candidates(repo: str, base_candidates: list[str], head_candidates: list[str]) -> tuple[str | None, str]:
|
||||
def _diff_with_candidates(
|
||||
repo: str, base_candidates: list[str], head_candidates: list[str]
|
||||
) -> tuple[str | None, str, list[tuple[str, str]]]:
|
||||
last_error = ""
|
||||
no_merge_base_pairs: list[tuple[str, str]] = []
|
||||
for base_ref in base_candidates:
|
||||
for head_ref in head_candidates:
|
||||
cmd = [
|
||||
"git",
|
||||
"-C",
|
||||
repo,
|
||||
"diff",
|
||||
f"{base_ref}...{head_ref}",
|
||||
"--unified=3",
|
||||
"--no-color",
|
||||
]
|
||||
result = subprocess.run(cmd, check=False, capture_output=True, text=True)
|
||||
result = _run_git_diff_command(repo, base_ref, head_ref, symmetric=True)
|
||||
if result.returncode in (0, 1):
|
||||
return result.stdout, ""
|
||||
return result.stdout, "", []
|
||||
|
||||
if result.stderr:
|
||||
last_error = result.stderr.strip()
|
||||
return None, last_error
|
||||
err = result.stderr.strip()
|
||||
last_error = err
|
||||
if "no merge base" in err.lower():
|
||||
no_merge_base_pairs.append((base_ref, head_ref))
|
||||
|
||||
return None, last_error, no_merge_base_pairs
|
||||
|
||||
|
||||
def parse_diff(diff_text: str) -> list[FileDiff]:
|
||||
@@ -193,3 +199,33 @@ def chunk_files(files: Iterable[FileDiff], max_lines: int = 350) -> list[DiffChu
|
||||
)
|
||||
|
||||
return chunks
|
||||
|
||||
|
||||
def _run_git_diff_command(
|
||||
repo: str, base_ref: str, head_ref: str, *, symmetric: bool
|
||||
) -> subprocess.CompletedProcess[str]:
|
||||
cmd = ["git", "-C", repo, "diff"]
|
||||
if symmetric:
|
||||
cmd.append(f"{base_ref}...{head_ref}")
|
||||
else:
|
||||
cmd.extend([base_ref, head_ref])
|
||||
cmd.extend(["--unified=3", "--no-color"])
|
||||
return subprocess.run(cmd, check=False, capture_output=True, text=True)
|
||||
|
||||
|
||||
def _fallback_diff_without_merge_base(
|
||||
repo: str, pairs: list[tuple[str, str]]
|
||||
) -> tuple[str | None, str]:
|
||||
last_error = ""
|
||||
seen: set[tuple[str, str]] = set()
|
||||
for base_ref, head_ref in pairs:
|
||||
key = (base_ref, head_ref)
|
||||
if key in seen:
|
||||
continue
|
||||
seen.add(key)
|
||||
result = _run_git_diff_command(repo, base_ref, head_ref, symmetric=False)
|
||||
if result.returncode in (0, 1):
|
||||
return result.stdout, ""
|
||||
if result.stderr:
|
||||
last_error = result.stderr.strip()
|
||||
return None, last_error
|
||||
|
||||
Binary file not shown.
@@ -38,8 +38,11 @@ def test_run_git_diff_falls_back_to_origin_refs_after_fetch():
|
||||
def fake_run(cmd, check=False, capture_output=False, text=False): # type: ignore[override]
|
||||
nonlocal fetched
|
||||
if cmd[3] == "diff":
|
||||
diff_attempts.append(cmd[4])
|
||||
if len(cmd) == 7:
|
||||
spec = cmd[4]
|
||||
else:
|
||||
spec = " ".join(cmd[4:6])
|
||||
diff_attempts.append(spec)
|
||||
if spec == "origin/base...origin/head" and fetched:
|
||||
return _completed(cmd, 0, stdout="remote-ok")
|
||||
return _completed(cmd, 128, stderr="missing ref")
|
||||
@@ -55,3 +58,29 @@ def test_run_git_diff_falls_back_to_origin_refs_after_fetch():
|
||||
assert "origin/base...origin/head" in diff_attempts
|
||||
# Ensure the fallback only succeeds after fetch.
|
||||
assert diff_attempts.index("origin/base...origin/head") > diff_attempts.index("base...head")
|
||||
|
||||
|
||||
def test_run_git_diff_fallbacks_to_two_dot_when_merge_base_missing():
|
||||
fetched = False
|
||||
diff_attempts: list[str] = []
|
||||
|
||||
def fake_run(cmd, check=False, capture_output=False, text=False): # type: ignore[override]
|
||||
nonlocal fetched
|
||||
if cmd[3] == "diff":
|
||||
if len(cmd) == 7:
|
||||
spec = cmd[4]
|
||||
diff_attempts.append(f"sym:{spec}")
|
||||
return _completed(cmd, 128, stderr="fatal: origin/main...origin/head: no merge base")
|
||||
spec = " ".join(cmd[4:6])
|
||||
diff_attempts.append(f"two:{spec}")
|
||||
return _completed(cmd, 0, stdout="linear-diff")
|
||||
if cmd[3] == "fetch":
|
||||
fetched = True
|
||||
return _completed(cmd, 0)
|
||||
raise AssertionError("unexpected git invocation")
|
||||
|
||||
with patch("ai_reviewer.diff.subprocess.run", side_effect=fake_run):
|
||||
output = run_git_diff(".", "base", "head")
|
||||
|
||||
assert output == "linear-diff"
|
||||
assert any(attempt.startswith("two:") for attempt in diff_attempts)
|
||||
|
||||
Reference in New Issue
Block a user