refactor: create list only if there's an error and update tests

Author: fabiovincenzi

src/proxy/processors/push-action/checkHiddenCommits.ts

@@ -8,67 +8,61 @@ const exec = async (req: any, action: Action): Promise<Action> => {
   try {
     const repoPath = `${action.proxyGitPath}/${action.repoName}`;
 
-    const introducedCommits = new Set<string>();
-
-    // Retrieve the single ref update
     const oldOid = action.commitFrom;
     const newOid = action.commitTo;
     if (!oldOid || !newOid) {
       throw new Error('Both action.commitFrom and action.commitTo must be defined');
     }
 
-    const revisionRange: string =
+    // build introducedCommits set
+    const introducedCommits = new Set<string>();
+    const revRange =
       oldOid === '0000000000000000000000000000000000000000' ? newOid : `${oldOid}..${newOid}`;
-
-    const revListOutput = spawnSync('git', ['rev-list', revisionRange], {
-      cwd: repoPath,
-      encoding: 'utf-8',
-    }).stdout;
-    revListOutput
-      .trim()
+    const revList = spawnSync('git', ['rev-list', revRange], { cwd: repoPath, encoding: 'utf-8' })
+      .stdout.trim()
       .split('\n')
-      .filter(Boolean)
-      .forEach((sha) => introducedCommits.add(sha));
+      .filter(Boolean);
+    revList.forEach((sha) => introducedCommits.add(sha));
     step.log(`Total introduced commits: ${introducedCommits.size}`);
-    const packPath = path.join('.git', 'objects', 'pack');
 
+    // build packCommits set
+    const packPath = path.join('.git', 'objects', 'pack');
     const packCommits = new Set<string>();
-
     (action.newIdxFiles || []).forEach((idxFile) => {
       const idxPath = path.join(packPath, idxFile);
       const out = spawnSync('git', ['verify-pack', '-v', idxPath], {
         cwd: repoPath,
         encoding: 'utf-8',
-      }).stdout;
-
-      out
-        .trim()
-        .split('\n')
-        .forEach((line) => {
-          const [sha, type] = line.split(/\s+/);
-          if (type === 'commit') packCommits.add(sha);
-        });
+      })
+        .stdout.trim()
+        .split('\n');
+      out.forEach((line) => {
+        const [sha, type] = line.split(/\s+/);
+        if (type === 'commit') packCommits.add(sha);
+      });
     });
-    step.log(`Commits nel pack: ${packCommits.size}`);
-    console.log('Pack commits:', packCommits);
+    step.log(`Total commits in the pack: ${packCommits.size}`);
 
-    const referenced: string[] = [];
-    const unreferenced: string[] = [];
-    [...packCommits].forEach((sha) => {
-      if (introducedCommits.has(sha)) referenced.push(sha);
-      else unreferenced.push(sha);
-    });
+    // subset check
+    const isSubset = [...packCommits].every((sha) => introducedCommits.has(sha));
+    if (!isSubset) {
+      // build detailed lists
+      const unreferenced = [...packCommits].filter((sha) => !introducedCommits.has(sha));
+      const referenced = [...packCommits].filter((sha) => introducedCommits.has(sha));
 
-    step.log(`✅ Referenced commits: ${referenced.length}`);
-    step.log(`❌ Unreferenced commits: ${unreferenced.length}`);
+      step.log(`✅ Referenced commits: ${referenced.length}`);
+      step.log(`❌ Unreferenced commits: ${unreferenced.length}`);
 
-    if (unreferenced.length > 0) {
       step.setError(
         `Unreferenced commits in pack (${unreferenced.length}): ${unreferenced.join(', ')}`,
       );
       action.error = true;
+      step.setContent(`Referenced: ${referenced.length}, Unreferenced: ${unreferenced.length}`);
+    } else {
+      // all good, no logging of individual SHAs needed
+      step.log('All pack commits are referenced in the introduced range.');
+      step.setContent(`All ${packCommits.size} pack commits are within introduced commits.`);
     }
-    step.setContent(`Referenced: ${referenced.length}, Unreferenced: ${unreferenced.length}`);
   } catch (e: any) {
     step.setError(e.message);
     throw e;

src/proxy/processors/push-action/writePack.ts

@@ -6,8 +6,6 @@ import fs from 'fs';
 const exec = async (req: any, action: Action) => {
   const step = new Step('writePack');
   try {
-    const cmd = `git receive-pack ${action.repoName}`;
-    step.log(`executing ${cmd}`);
     if (!action.proxyGitPath || !action.repoName) {
       throw new Error('proxyGitPath and repoName must be defined');
     }

test/checkHiddenCommit.test.js

@@ -32,28 +32,46 @@ describe('checkHiddenCommits.exec', () => {
     sandbox.restore();
   });
 
-  it('reports all commits unreferenced and sets error=true', async () => {
+  it.only('reports all commits unreferenced and sets error=true', async () => {
+    const COMMIT_1 = 'deadbeef';
+    const COMMIT_2 = 'cafebabe';
+
     // 1) rev-list → no introduced commits
     // 2) verify-pack → two commits in pack
-    spawnSyncStub.onFirstCall().returns({ stdout: '' }).onSecondCall().returns({
-      stdout: 'deadbeef commit 100 1\ncafebabe commit 100 2\n',
-    });
+    spawnSyncStub
+      .onFirstCall()
+      .returns({ stdout: '' })
+      .onSecondCall()
+      .returns({
+        stdout: `${COMMIT_1} commit 100 1\n${COMMIT_2} commit 100 2\n`,
+      });
 
     readdirSyncStub.returns(['pack-test.idx']);
 
     await checkHidden({ body: '' }, action);
 
     const step = action.steps.find((s) => s.stepName === 'checkHiddenCommits');
-    expect(step.logs).to.include('checkHiddenCommits - ❌ Unreferenced commits: 2');
+    expect(step.logs).to.include(`checkHiddenCommits - ✅ Referenced commits: 0`);
+    expect(step.logs).to.include(`checkHiddenCommits - ❌ Unreferenced commits: 2`);
+    expect(step.logs).to.include(
+      `checkHiddenCommits - Unreferenced commits in pack (2): ${COMMIT_1}, ${COMMIT_2}`,
+    );
     expect(action.error).to.be.true;
   });
 
-  it('mixes referenced & unreferenced correctly', async () => {
+  it.only('mixes referenced & unreferenced correctly', async () => {
+    const COMMIT_1 = 'deadbeef';
+    const COMMIT_2 = 'cafebabe';
+
     // 1) git rev-list → introduces one commit “deadbeef”
     // 2) git verify-pack → the pack contains two commits
-    spawnSyncStub.onFirstCall().returns({ stdout: 'deadbeef\n' }).onSecondCall().returns({
-      stdout: 'deadbeef commit 100 1\ncafebabe commit 100 2\n',
-    });
+    spawnSyncStub
+      .onFirstCall()
+      .returns({ stdout: `${COMMIT_1}\n` })
+      .onSecondCall()
+      .returns({
+        stdout: `${COMMIT_1} commit 100 1\n${COMMIT_2} commit 100 2\n`,
+      });
 
     readdirSyncStub.returns(['pack-test.idx']);
 
@@ -62,6 +80,9 @@ describe('checkHiddenCommits.exec', () => {
     const step = action.steps.find((s) => s.stepName === 'checkHiddenCommits');
     expect(step.logs).to.include('checkHiddenCommits - ✅ Referenced commits: 1');
     expect(step.logs).to.include('checkHiddenCommits - ❌ Unreferenced commits: 1');
+    expect(step.logs).to.include(
+      `checkHiddenCommits - Unreferenced commits in pack (1): ${COMMIT_2}`,
+    );
     expect(action.error).to.be.true;
   });
 
@@ -75,14 +96,17 @@ describe('checkHiddenCommits.exec', () => {
     readdirSyncStub.returns(['pack-test.idx']);
 
     await checkHidden({ body: '' }, action);
-
     const step = action.steps.find((s) => s.stepName === 'checkHiddenCommits');
-    expect(step.logs).to.include('checkHiddenCommits - ✅ Referenced commits: 2');
-    expect(step.logs).to.include('checkHiddenCommits - ❌ Unreferenced commits: 0');
+
+    expect(step.logs).to.include('checkHiddenCommits - Total introduced commits: 2');
+    expect(step.logs).to.include('checkHiddenCommits - Total commits in the pack: 2');
+    expect(step.logs).to.include(
+      'checkHiddenCommits - All pack commits are referenced in the introduced range.',
+    );
     expect(action.error).to.be.false;
   });
 
-  it('throws if commitFrom or commitTo is missing', async () => {
+  it.only('throws if commitFrom or commitTo is missing', async () => {
     delete action.commitFrom;
 
     try {