Skip to content

Commit fcae2db

Browse files
committed
fix(github-code-review): remove --3way from git apply strategies, fix resetCheckoutToCleanState
Root cause: gh repo clone creates refs and index files but the actual .pack data can be incomplete/missing, leaving the git object store with missing commit objects. This causes "bad object HEAD" errors and prevents git apply --3way from working (--3way needs base commit objects for 3-way merge). Changes: applyPRDiff strategy order (simplified): - git apply --whitespace=nowarn ← first choice (no --3way needed, base files exist because we cloned the base branch) - git apply --whitespace=fix - git apply --whitespace=nowarn --ignore-whitespace - git apply --binary --whitespace=nowarn ← for binary file changes - patch -p1 --no-backup-if-mismatch ← last resort, most lenient Removed: - patch --dry-run → patch two-step (dry-run succeeds but hunks are ignored in incomplete clones, making dry-run unreliable) - All --3way flags (requires base commit objects in object store) resetCheckoutToCleanState: - Changed from "git reset --hard HEAD && git checkout" to "git checkout -f" which does NOT require HEAD commit object to exist in object store — prevents "bad object HEAD" failures in incomplete clones. Also cleaned broken checkout at /tmp/pr-checkouts/interserver/my/158 (which had missing .pack file) to force fresh clone on next run. Verification: - php -l: no errors - PHPStan level 8: 0 errors - Worker tests: 75/75 pass - CLI tests: 232/232 pass
1 parent 23bb204 commit fcae2db

1 file changed

Lines changed: 18 additions & 56 deletions

File tree

scripts/github-code-review.php

Lines changed: 18 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -478,76 +478,36 @@ function applyPRDiff(string $checkoutPath, ?string $diff): bool
478478
// Try multiple strategies in order of preference
479479
// Strategy order: most reliable first, least desperate last
480480
$strategies = [
481-
// Strategy 1: patch --dry-run to validate, then patch for real if validation passes
482-
// Split into two steps so we don't run real patch as part of same && chain
481+
// Strategy 1: plain git apply — most reliable when base files exist (they do, we cloned the base branch)
483482
[
484-
'label' => 'patch-dryrun',
483+
'cmd' => 'git apply --whitespace=nowarn %s 2>&1',
484+
'label' => 'git-apply',
485485
],
486-
// Strategy 2: git apply with --3way (requires base in repo for best results)
487-
[
488-
'cmd' => 'git apply --3way --whitespace=nowarn %s 2>&1',
489-
'label' => 'git-apply-3way',
490-
],
491-
// Strategy 3: git apply with --whitespace=fix (auto-fix whitespace issues)
486+
// Strategy 2: git apply with --whitespace=fix (auto-fix whitespace issues)
492487
[
493488
'cmd' => 'git apply --whitespace=fix %s 2>&1',
494489
'label' => 'git-apply-whitespace-fix',
495490
],
496-
// Strategy 4: git apply binary (handles binary file mode changes)
497-
[
498-
'cmd' => 'git apply --binary --3way --whitespace=nowarn %s 2>&1',
499-
'label' => 'git-apply-binary-3way',
500-
],
501-
// Strategy 5: plain git apply ignoring whitespace
491+
// Strategy 3: git apply ignoring whitespace (handles context drift)
502492
[
503493
'cmd' => 'git apply --whitespace=nowarn --ignore-whitespace %s 2>&1',
504494
'label' => 'git-apply-ignorews',
505495
],
496+
// Strategy 4: git apply binary (handles binary file mode changes in the diff)
497+
[
498+
'cmd' => 'git apply --binary --whitespace=nowarn %s 2>&1',
499+
'label' => 'git-apply-binary',
500+
],
501+
// Strategy 5: patch as last resort — most lenient, handles binary, long lines, etc.
502+
[
503+
'cmd' => 'patch -p1 --no-backup-if-mismatch < %s 2>&1',
504+
'label' => 'patch',
505+
],
506506
];
507507

508508
$lastOutput = '';
509509

510510
foreach ($strategies as $strategy) {
511-
// Strategy 1: separate dry-run step, then real patch only if dry-run succeeded
512-
if ($strategy['label'] === 'patch-dryrun') {
513-
$dryRunCmd = sprintf(
514-
'cd %s && patch -p1 --no-backup-if-mismatch --dry-run < %s 2>&1',
515-
escapeshellarg($checkoutPath),
516-
escapeshellarg($diffFile)
517-
);
518-
$output = [];
519-
$ret = 0;
520-
exec($dryRunCmd, $output, $ret);
521-
$dryRunOutput = implode("\n", $output);
522-
523-
if ($ret !== 0) {
524-
verbose_log("applyPRDiff: patch --dry-run failed: " . substr($dryRunOutput, 0, 200), 3);
525-
continue; // try next strategy
526-
}
527-
528-
// Dry-run passed — now apply for real
529-
verbose_log("applyPRDiff: patch --dry-run passed, applying for real...", 3);
530-
$applyCmd = sprintf(
531-
'cd %s && patch -p1 --no-backup-if-mismatch < %s 2>&1',
532-
escapeshellarg($checkoutPath),
533-
escapeshellarg($diffFile)
534-
);
535-
$output = [];
536-
$ret = 0;
537-
exec($applyCmd, $output, $ret);
538-
$applyOutput = implode("\n", $output);
539-
540-
if ($ret === 0) {
541-
verbose_log("applyPRDiff: diff applied successfully (strategy: patch)", 3);
542-
return true;
543-
}
544-
545-
verbose_log("applyPRDiff: patch --dry-run passed but real patch failed: " . substr($applyOutput, 0, 200), 3);
546-
$lastOutput = $applyOutput;
547-
continue; // try next strategy
548-
}
549-
550-
// Remaining strategies use a single command
551511
$cmd = sprintf(
552512
'cd %s && ' . $strategy['cmd'],
553513
escapeshellarg($checkoutPath),
@@ -1336,8 +1296,10 @@ function invalidateCachedCheckout(string $repo, string $branch): void
13361296
*/
13371297
function resetCheckoutToCleanState(string $checkoutPath, string $baseBranch): bool
13381298
{
1299+
// Use checkout -f instead of reset --hard HEAD to avoid requiring HEAD commit
1300+
// object to exist in object store (important for incomplete/shallow clones)
13391301
$cmd = sprintf(
1340-
'cd %s && git reset --hard HEAD 2>&1 && git checkout %s 2>&1',
1302+
'cd %s && git checkout -f %s 2>&1',
13411303
escapeshellarg($checkoutPath),
13421304
escapeshellarg($baseBranch)
13431305
);

0 commit comments

Comments
 (0)