Skip to content

jscpd copy-paste scan: two real duplications worth extracting, the rest is boilerplate the 0% threshold is too strict for #112

Description

@laywill

jscpd 5.0.14 finds 11 clone pairs (0.58% of lines, 0.86% of tokens) and fails outright because .mega-linter.yml's generated jscpd config sets "threshold": 0 — any duplication at all is an error. Of the 11, two are genuine copy-paste in shipped code worth extracting into a helper; the rest is either cross-heredoc boilerplate that can't be deduplicated without adding real complexity, or test-fixture repetition that's inherent to how this repo's bash tests are structured. None of it looks like it happened by accident.

jscpd 5.0.14 raw output
Clone found (bash)
 - deploy.sh [28:1 - 34:3] (7 lines, 85 tokens)
   deploy.sh [89:1 - 95:3]
Clone found (bash)
 - deploy.sh [205:5 - 220:7] (16 lines, 101 tokens)
   install-dev.sh [479:5 - 494:7]
Clone found (bash)
 - install-dev.sh [197:48 - 204:2] (8 lines, 60 tokens)
   install-dev.sh [408:46 - 415:2]
Clone found (bash)
 - install-dev.sh [325:1 - 335:2] (11 lines, 73 tokens)
   install-dev.sh [517:1 - 527:2]
Clone found (php)
 - src/usr/local/emhttp/plugins/ci-runner-farm/include/exec.php [332:31 - 337:6] (6 lines, 67 tokens)
   src/usr/local/emhttp/plugins/ci-runner-farm/include/exec.php [348:33 - 353:6]
Clone found (bash)
 - src/usr/local/emhttp/plugins/ci-runner-farm/include/runner-farm.sh [496:3 - 501:45] (6 lines, 55 tokens)
   src/usr/local/emhttp/plugins/ci-runner-farm/include/runner-farm.sh [1910:3 - 1915:45]
Clone found (bash)
 - src/usr/local/emhttp/plugins/ci-runner-farm/include/runner-farm.sh [2190:11 - 2195:63] (6 lines, 64 tokens)
   src/usr/local/emhttp/plugins/ci-runner-farm/include/runner-farm.sh [3474:19 - 3479:63]
Clone found (bash)
 - tests/gitlab-policy.sh [4:1 - 14:74] (11 lines, 57 tokens)
   tests/provider-mocks.sh [4:1 - 14:74]
Clone found (bash)
 - tests/log-redaction.sh [89:23 - 94:4] (6 lines, 53 tokens)
   tests/log-redaction.sh [107:31 - 112:4]
Clone found (bash)
 - tests/provider-mocks.sh [563:1 - 570:8] (8 lines, 54 tokens)
   tests/provider-mocks.sh [584:1 - 591:8]
Clone found (bash)
 - tests/provider-mocks.sh [875:104 - 881:37] (7 lines, 55 tokens)
   tests/provider-mocks.sh [890:60 - 896:37]

bash: 42 files, 13258 lines, 10 clones, 76 duplicated lines (0.57%)
php:  3 files, 733 lines, 1 clone, 5 duplicated lines (0.68%)
Total: 45 files, 13991 lines, 11 clones, 81 duplicated lines (0.58%), 724 duplicated tokens (0.86%)

Worth fixing: runner-farm.sh's managed-runner loop header

Four functions in the engine — count_stale_runners, reconcile_stale_runners, quiesce_gitlab_managers_for_stop, and cmd_docker_stopping_locked — each open with the identical five lines that list managed runners and decode one snapshot:

# runner-farm.sh:496-500 (count_stale_runners) and :1910-1914 (reconcile_stale_runners)
  names="$(managed_names)" || return 1
  for c in $names; do
    [ -n "$c" ] || continue
    snapshot="$(managed_runner_snapshot "$c")" || return 1
    IFS='|' read -r id provider role index gen <<< "$snapshot"
# runner-farm.sh:2190-2195 (quiesce_gitlab_managers_for_stop) and :3474-3479 (cmd_docker_stopping_locked)
  names="$(managed_names)" || return 1
  for c in $names; do
    [ -n "$c" ] || continue
    snapshot="$(managed_runner_snapshot "$c")" || return 1
    IFS='|' read -r id provider role index gen <<< "$snapshot"

jscpd reports these as two separate clone pairs rather than one four-way clique because the preceding local declaration line differs between the two groups (the quiesce/cmd_docker_stopping_locked pair also tracks pids/failed/count for parallel signaling), which shifts where each match starts — but all four sites are the same "list managed runners, decode one snapshot" idiom. A small helper removes the duplication without changing what each caller does with the result afterward:

# in runner-farm.sh, near managed_runner_snapshot's definition
read_managed_snapshot() {
  snapshot="$(managed_runner_snapshot "$1")" || return 1
  IFS='|' read -r id provider role index gen <<< "$snapshot"
}

Each of the four sites then calls read_managed_snapshot "$c" || return 1 in place of its two duplicated lines. This is small (four call sites, ~3400-line file, 0.6% of it), but it's real production-code duplication with no structural reason to keep it duplicated, unlike the rest of this report.

Worth fixing: exec.php's pool-id validation

# exec.php:332-337 (set-gitlab-pool-token)
    $pool = $_POST['pool'] ?? '';
    if (!is_string($pool) || !preg_match('/^[a-z](?:[a-z0-9-]{0,22}[a-z0-9])?$/D', $pool)
        || in_array($pool, ['default', 'invalid'], true)) {
      echo crf_json(['ok'=>false,'error'=>'invalid pool id']); break;
    }
# exec.php:348-353 (clear-gitlab-pool-token)
    $pool = $_POST['pool'] ?? '';
    if (!is_string($pool) || !preg_match('/^[a-z](?:[a-z0-9-]{0,22}[a-z0-9])?$/D', $pool)
        || in_array($pool, ['default', 'invalid'], true)) {
      echo crf_json(['ok'=>false,'error'=>'invalid pool id']); break;
    }

Byte-for-byte identical, six lines, in the same file. A one-line helper closes this:

function valid_pool_id($pool) {
  return is_string($pool)
    && preg_match('/^[a-z](?:[a-z0-9-]{0,22}[a-z0-9])?$/D', $pool)
    && !in_array($pool, ['default', 'invalid'], true);
}

with both call sites reduced to if (!valid_pool_id($pool)) { echo crf_json(['ok'=>false,'error'=>'invalid pool id']); break; }. Only these two cases exist today (grep -n "_POST\['pool'\]" exec.php turns up nothing else), but it stops the validation rule from drifting between call sites if a third pool-scoped action gets added later.

Not worth fixing: deploy.sh and install-dev.sh's local/remote pairs

# deploy.sh:28-34 (local pre-flight check) and :89-95 (the same check, re-run after the tar lands on the remote host)
for required in \
  RunnerFarm.page RunnerFarmDashboard.page RunnerFarmFleet.page RunnerFarmImage.page RunnerFarmSettings.page \
  default.cfg default.Dockerfile default.github.Dockerfile default.gitlab.Dockerfile \
  ...

This one's real (the file list has to be kept in sync by hand) but it's checking two different things at two different points: the local copy before it's sent, and the staged copy after tar lands it over SSH. The second check runs inside a bash -s -- ... <<'REMOTE' heredoc with a quoted delimiter, so it can't reference a shell array defined in the parent script — it's a separate script body sent verbatim to a remote shell. Deduplicating it means either building the file list once and interpolating it into the heredoc (workable, but requires switching the heredoc to allow expansion, which then has to be re-audited for accidental expansion of anything else in the remote script), or writing the list to a temp file and having the remote script read it back. Either is a real change in risk profile for a script whose entire job is "install this safely, with rollback" — I'd rather flag the duplication for awareness than touch it.

The deploy.sh:205-220 / install-dev.sh:479-494 pair and install-dev.sh:197-204 / install-dev.sh:408-415 pair are the same shape: fail/sha256_of/entity_value/regular_file helper definitions duplicated between deploy.sh and two separate <<'REMOTE_INSTALL' / <<'REMOTE_ROLLBACK' heredocs in install-dev.sh (confirmed at install-dev.sh:188 and :404 — two independent SSH invocations, each carrying a self-contained script). Same reasoning: real text duplication, but the two copies execute in isolated remote-shell contexts that don't share process state, so removing it means shipping a small shared library file to the remote host first and sourcing it from both, which is more moving parts than the 8-19 duplicated lines it saves.

The install-dev.sh:325-335 / :517-527 pair (known_dev_descriptor(), defined once in the install heredoc and once in the rollback heredoc) is the same story — different heredoc, different SSH invocation, can't share a function definition without a shared remote file.

Not worth fixing: test fixture repetition

# tests/gitlab-policy.sh:4-14 and tests/provider-mocks.sh:4-14
set -euo pipefail
cd "$(dirname "$0")/.."

tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
export CRF_CFGDIR="$tmp/config"
export CRF_RUNDIR="$tmp/run"
export CRF_SOURCE_ONLY=1
mkdir -p "$CRF_CFGDIR" "$CRF_RUNDIR"
# shellcheck source=/dev/null
source src/usr/local/emhttp/plugins/ci-runner-farm/include/runner-farm.sh

This is the standard per-file test harness preamble (tests/check.sh runs each test script standalone, there's no shared test framework), and it's almost certainly not limited to just these two files — it's the setup every CRF_SOURCE_ONLY=1 test in tests/ needs. Extracting it into a sourced tests/lib/harness.sh is possible, but it cuts against the repo's existing style of small, independently readable test scripts with no shared runtime beyond runner-farm.sh itself, and jscpd's 11-line/57-token match is below what most people would call meaningful duplication for a setup block. I'd allowlist this rather than restructure the test suite around it.

The remaining two tests/provider-mocks.sh pairs and the tests/log-redaction.sh pair are the same kind of thing at a smaller scale: tests/provider-mocks.sh:563-570/:584-591 are two GitLab DinD-vs-host-socket preflight subshells that redefine the same four mock functions on purpose, because each subshell needs its own isolated stubs to prove the guard behaves differently under DIND=true/DIND=false; tests/provider-mocks.sh:875-881/:890-896 and tests/log-redaction.sh:89-94/:107-112 are near-identical python3 -c/php -r assertion blocks checking a different field set or scenario each time. None of these are duplicated logic so much as duplicated test scaffolding around distinct scenarios.

Suggested fix

Fix the two runner-farm.sh/exec.php clusters above, then add a repo-level .jscpd.json (MegaLinter's generated megalinter-reports/jscpd-config.json currently sets "threshold": 0, with no project override) that raises the threshold enough to tolerate the remaining structural/test duplication rather than chasing it to zero:

{
  "threshold": 0.5,
  "ignore": ["**/node_modules/**", "tests/**"]
}

0.5% is a placeholder — after the two real fixes land, total duplication drops from 0.86% to roughly 0.5% of tokens by my count of the remaining 9 clone pairs, so the exact number needs re-checking against a real jscpd run once those two PRs are in rather than trusted from my arithmetic here. Excluding tests/** from jscpd entirely is the blunter alternative if the maintainer would rather not tune a threshold number at all.

I can open a PR for the two real fixes; the threshold/ignore call is a judgment one I'd want agreement on first.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions