akiomik / akiomik/mado

setup.sh can empty mado's working tree

Open
#399 5 comments 0 reactions 0 assignees View on GitHub
bug ci
Dominant language
Rust
Stars
407
Forks
12
Avg merge
5h 45m
Merged PRs (30d)
30

Description

Split out of #396, which was narrowed to the bugs that stop the development
scripts from doing what they say. This is a different thing: a data-loss bug,
and one that took four attempts to close.

## What happens

`scripts/acceptance/setup.sh` and `scripts/benchmarks/setup.sh` both `cd` into
a directory under `scripts/*/data` and run `git sparse-checkout set` there.
`git` decides which repository it acts on by walking up from the working
directory, so when that directory is not a repository — a clone that failed
offline, one interrupted after creating the directory, a stray `mkdir` — the
command reaches **mado's own checkout** and strips every subdirectory from the
working tree.

Reproduced against a scratch repository with `git clone` forced to fail:

| | exit | working tree afterwards |
| --- | --- | --- |
| `benchmarks/setup.sh` | **0** | `src/`, `tests/` gone |
| `acceptance/setup.sh` | 1 | `src/`, `tests/` gone |

`benchmarks/setup.sh` is the worse of the two: mado has no `doc/`, so
`sparse-checkout set doc` against it leaves nothing but the root files, and it
reports success. Recoverable with `git sparse-checkout disable`, but nothing
tells the user what happened.

## Why this needs a design decision rather than a patch

Four defences were written for this during the review of #396. The first three
were all wrong, each for a reason the previous one did not suggest:

| defence | how it fails |
| --- | --- |
| `cd "$CLONE_DIR" \|\| exit 1` | `cd` succeeds on a directory that exists but holds no repository |
| check `style_files` is non-empty afterwards | reports the damage instead of preventing it |
| `[ -d "$CLONE_DIR/.git" ]` | an empty `.git` is not a repository to git either, and the search walks past it |
| `GIT_CEILING_DIRECTORIES="$DATA_ROOT"` | does not apply to an inherited `GIT_DIR`, which a git hook, `git bisect run` and `git rebase --exec` all export |

The fourth — `unset GIT_DIR GIT_WORK_TREE` plus `--git-dir` and `--work-tree`
on every command — held in testing, including with `GIT_DIR` exported. But a
defence that took four tries is a sign the shape is wrong, not that the fourth
one is right. Worth considering instead:

- Clone with `--depth 1` and no sparse checkout, so `sparse-checkout set` is
never run at all.
- Clone outside the mado working tree entirely, so there is no parent
repository to walk up into.
- Do not express this in shell — see #397, which asks the same question about
`action/entrypoint.sh`.

## Why the linter would not have caught it

`shellcheck` reports `SC2164` ("use `cd ... || exit`") on these lines, and
taking that advice produces the *first* defence in the table above — the one
that does not work. Nothing static knows that `git` walks up, or that
`GIT_CEILING_DIRECTORIES` has an exception for `GIT_DIR`. This is the limit of
the tooling under discussion in #398, not an argument against adopting it.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.