mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 06:06:37 +03:00
Accept remote refs as diff bases; honor non-origin upstreams
Both review bots found real gaps in the first pass: candidates were verified as local branch names only, so an origin/HEAD target with no local checkout fell through, and stripOrigin() dropped upstreams on any remote not named origin (fork workflows tracking upstream/release). Candidates now carry a display name plus the revs to try in order: the upstream's remote rev wins outright (it tracks the actual merge target, so it beats a possibly stale local branch of the same name), origin/HEAD tries the local branch then the remote-tracking ref, and the conventional names each try local then origin/<name>. git.base keeps reporting the friendly branch name while the diff runs against whichever rev resolved. Two new failing-first tests: remote-only default branch, and an upstream on a remote named upstream with no local base branch. Prepared with AI assistance (Claude Code), directed by @pbakaus. Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Code
parent
a50702f2b6
commit
ea098ceb96
@@ -96,24 +96,43 @@ function gitSignals(cwd) {
|
||||
// are withheld when the current branch IS one of them: sitting on main
|
||||
// in a repo that also has develop must not diff the two integration
|
||||
// branches against each other.
|
||||
const stripOrigin = (ref) => (ref && ref.startsWith('origin/') ? ref.slice('origin/'.length) : null);
|
||||
const upstreamBase = stripOrigin(run(['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}']));
|
||||
const remoteHeadBase = stripOrigin(run(['symbolic-ref', '--short', 'refs/remotes/origin/HEAD']));
|
||||
// Candidates carry a display name (what git.base reports) and the revs to
|
||||
// try, in order. A remote ref like `upstream/release` (fork workflows) or
|
||||
// an origin/HEAD target with no local checkout is a perfectly good diff
|
||||
// base, so revs are not limited to local branch names.
|
||||
const splitRemoteRef = (ref) => {
|
||||
const i = ref ? ref.indexOf('/') : -1;
|
||||
return i > 0 ? { name: ref.slice(i + 1), rev: ref } : null;
|
||||
};
|
||||
const upstream = splitRemoteRef(run(['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}']));
|
||||
const remoteHead = splitRemoteRef(run(['symbolic-ref', '--short', 'refs/remotes/origin/HEAD']));
|
||||
const conventional = ['develop', 'main', 'master'];
|
||||
const candidates = new Set([
|
||||
...[upstreamBase, remoteHeadBase].filter(Boolean),
|
||||
...(conventional.includes(branch) ? [] : conventional),
|
||||
]);
|
||||
const candidates = [];
|
||||
const seen = new Set();
|
||||
const addCandidate = (name, revs) => {
|
||||
if (!name || name === branch || seen.has(name)) return;
|
||||
seen.add(name);
|
||||
candidates.push({ name, revs });
|
||||
};
|
||||
// The upstream tracks the actual merge target, so its remote rev wins over
|
||||
// a possibly stale local branch of the same name.
|
||||
if (upstream) addCandidate(upstream.name, [upstream.rev]);
|
||||
if (remoteHead) addCandidate(remoteHead.name, [remoteHead.name, remoteHead.rev]);
|
||||
if (!conventional.includes(branch)) {
|
||||
for (const name of conventional) addCandidate(name, [name, `origin/${name}`]);
|
||||
}
|
||||
let base = null;
|
||||
for (const b of candidates) {
|
||||
if (b === branch) continue;
|
||||
if (run(['rev-parse', '--verify', '--quiet', b]) !== null) {
|
||||
base = b;
|
||||
let baseRev = null;
|
||||
for (const c of candidates) {
|
||||
const rev = c.revs.find((r) => run(['rev-parse', '--verify', '--quiet', r]) !== null);
|
||||
if (rev) {
|
||||
base = c.name;
|
||||
baseRev = rev;
|
||||
break;
|
||||
}
|
||||
}
|
||||
const diffBase = base && branch && branch !== base ? base : null;
|
||||
const fromDiff = diffBase ? run(['diff', '--name-only', `${diffBase}...HEAD`]) : null;
|
||||
const fromDiff = diffBase ? run(['diff', '--name-only', `${baseRev}...HEAD`]) : null;
|
||||
// porcelain lines are `XY PATH`: a 2-char status + a space, then the path.
|
||||
// Don't trim the combined output — an unstaged-modified line starts with a
|
||||
// leading space (` M path`), and a global trim would eat the first line's
|
||||
|
||||
@@ -266,6 +266,49 @@ describe('gatherSignals', () => {
|
||||
assert.deepEqual(s.git.changedFiles, ['src/App.tsx']);
|
||||
});
|
||||
|
||||
it('uses the remote-tracking ref when the base has no local branch (#302)', async () => {
|
||||
const { execFileSync } = await import('node:child_process');
|
||||
const git = (...args) => execFileSync('git', args, { cwd: scratch, stdio: 'ignore' });
|
||||
git('init', '-q', '-b', 'develop');
|
||||
git('config', 'user.email', 't@example.com');
|
||||
git('config', 'user.name', 'Test');
|
||||
write('src/App.tsx', 'export default 1;\n');
|
||||
git('add', '.');
|
||||
git('commit', '-qm', 'init');
|
||||
git('update-ref', 'refs/remotes/origin/develop', 'HEAD');
|
||||
git('symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/develop');
|
||||
git('checkout', '-q', '-b', 'feature/w');
|
||||
git('branch', '-q', '-D', 'develop'); // remote default exists, local doesn't
|
||||
write('src/Hero.tsx', 'export const Hero = () => null;\n');
|
||||
git('add', '.');
|
||||
git('commit', '-qm', 'feature work');
|
||||
const s = await gatherSignals(scratch);
|
||||
assert.equal(s.git.base, 'develop');
|
||||
assert.deepEqual(s.git.changedFiles, ['src/Hero.tsx']);
|
||||
});
|
||||
|
||||
it('honors an upstream on a non-origin remote (fork workflow) (#302)', async () => {
|
||||
const { execFileSync } = await import('node:child_process');
|
||||
const git = (...args) => execFileSync('git', args, { cwd: scratch, stdio: 'ignore' });
|
||||
git('init', '-q', '-b', 'release');
|
||||
git('config', 'user.email', 't@example.com');
|
||||
git('config', 'user.name', 'Test');
|
||||
write('src/App.tsx', 'export default 1;\n');
|
||||
git('add', '.');
|
||||
git('commit', '-qm', 'init');
|
||||
git('remote', 'add', 'upstream', '.');
|
||||
git('update-ref', 'refs/remotes/upstream/release', 'HEAD');
|
||||
git('checkout', '-q', '-b', 'feature/v');
|
||||
git('branch', '-q', '--set-upstream-to=upstream/release');
|
||||
git('branch', '-q', '-D', 'release'); // the tracked base lives only on the fork parent
|
||||
write('src/Hero.tsx', 'export const Hero = () => null;\n');
|
||||
git('add', '.');
|
||||
git('commit', '-qm', 'feature work');
|
||||
const s = await gatherSignals(scratch);
|
||||
assert.equal(s.git.base, 'release');
|
||||
assert.deepEqual(s.git.changedFiles, ['src/Hero.tsx']);
|
||||
});
|
||||
|
||||
it('never diffs one integration branch against another (#302)', async () => {
|
||||
const { execFileSync } = await import('node:child_process');
|
||||
const git = (...args) => execFileSync('git', args, { cwd: scratch, stdio: 'ignore' });
|
||||
|
||||
Reference in New Issue
Block a user