fix: make gitlab-backup importable and testable, and fix what that surfaced - #227
Merged
Conversation
gitlab_backup/ held only __init__.py while all 418 lines of logic lived in the bin script, so nothing was importable and nothing could be tested. The logic now lives in gitlab_backup/gitlab_backup.py, CLI wiring in gitlab_backup/cli.py, and bin/gitlab-backup is a thin backwards-compatible wrapper. Installation switches from scripts= to a console_scripts entry point, and the package can also be run with python -m gitlab_backup. Carried over from python-github-backup while moving: - the hand-rolled log_* helpers become a module level logger, so output can be filtered with --quiet and --log-level - logging_subprocess drains stdout and stderr from threads instead of select(), fixing a deadlock once a child wrote more than the pipe buffer and a truncated final line - parse_args accepts an explicit argv, which is what makes it testable
The refactor made the code importable; this covers it. 86 tests across the helpers, argument parsing, client construction, and the clone and update flows, with subprocess and python-gitlab mocked so nothing touches a network. conftest builds args from the real parser, so a new flag arrives in every test with its actual default and no test needs editing. Runs on 3.10 through 3.14 in CI. pytest and its dependencies are pinned in release-requirements.txt alongside the existing tooling, and tests are kept out of the docker build context.
Every logging_subprocess return code was discarded, so a run where all 200 clones failed still exited 0 and nothing downstream could tell a working backup from a broken one. A new run_git raises GitCommandError on a non-zero exit, and main collects failures per repository: one bad repository no longer costs the other 199, but the run ends non-zero and names what failed. git ls-remote answers 0 with no refs for an empty-but-existing repository, so exit code 128 never meant "not initialized" as the old message claimed. It means the remote could not be read: missing, private, or bad credentials. With an expired token every repository was being skipped under that message and the backup reported success.
The LFS fetch sat in the else branch of the ref fetch, so --clone-lfs replaced it. An existing clone never received new commits, branches or tags again: the backup looked healthy while going quietly stale. A first clone also never pulled LFS content at all, leaving it a run behind. git fetch now always runs and the LFS fetch follows it on both the clone and the update path, matching python-github-backup. --clone-lfs also checks that git-lfs is installed before starting, rather than producing a backup with no LFS content in it.
The credential was passed to git as -c http.extraHeader=..., which any local user could read from ps while a clone ran. It also went to stderr verbatim whenever a git command failed, so an expired-looking base64 blob of oauth2:<token> ended up in CI logs and cron mail. It now travels in GIT_CONFIG_KEY_n/GIT_CONFIG_VALUE_n instead, appended after any entries the caller already set, so it never appears in argv. This needs git 2.31 or newer. Anything still printed goes through mask_command first, which redacts the header however it is spelled and any password embedded in a remote URL. mask_password now rewrites only the userinfo part of the URL. It used to replace every occurrence of the password text, so a password that also appeared in the host or path blanked those too and the log line no longer said which repository had failed.
Nothing limited how long a git command could run, so a dead connection hung the backup indefinitely and an unattended run never finished. Two limits, because they solve different failures. --stall-timeout, on by default at 60 seconds, sets git's own http.lowSpeedLimit and lowSpeedTime so a transfer making no progress is abandoned while a large but healthy download is left alone. --git-timeout is a hard per-command cap, off by default so no legitimately long clone starts failing; the child is killed and reported. ls-remote gets the same treatment through probe_remote. subprocess.call would have raised TimeoutExpired and left the hung git running past the backup. --retries, 3 by default, retries a failed command with a widening pause. Only runs that were already failing are affected. A clone is retried only while the target path is still clear, since git removes a directory it created but a retry onto an occupied path would fail differently. All three reject negative values, which would otherwise expire immediately and kill every command the moment it started. git writes ordinary progress to stderr, so those lines are logged at debug level now and only quoted back, tail first, when a command actually fails.
The update path was taken whenever a .git entry existed, which said nothing about whether git could read it. A run killed part way through a clone leaves exactly that: a directory git rejects, which then failed on every later run with a confusing error and never recovered on its own. check_existing_clone asks git where its repository actually is and accepts it only when that is exactly where this clone should have put one. Comparing the path matters: git discovery walks up the directory tree, so backing up into a directory nested inside another repository would otherwise look like our clone, and we would rewrite that repository's origin and fetch over it with --force --tags --prune. A directory git cannot read is now reported as an interrupted clone, and one that was never a repository as exactly that. A clone this run created and then failed is removed so the next attempt starts clean; a directory that existed beforehand is never touched.
mhajder
marked this pull request as draft
August 16, 2026 17:42
On a case-insensitive filesystem Group/Project and group/project resolve to one directory, so the second project rewrote the first's origin and pulled a different repository's refs into it. Two backups silently merged into one. Guarded at both ends. Before cloning, projects whose paths differ only in capitalisation are reported and skipped, but only when the output directory really is case-insensitive: on ext4 they are distinct directories and both back up correctly, so the check probes the filesystem rather than assuming. Before any origin is rewritten, the existing clone's remote is compared with the incoming one. Host and project path are compared, not the whole URL, so switching between https and ssh still updates in place while a different project, or the same path on another instance, is refused. --allow-host-change covers a renamed instance. Projects the listing cannot describe no longer abort the run: they are reported and counted as failures like any other, rather than raising out of the loop before anything is backed up.
--namespace group matched only projects sitting directly in that group, so a group organised into subgroups backed up nothing at all while reporting success. The help text promised a namespace, which reads as everything under it. Matching is now on a path boundary, so group also takes group/subgroup and deeper, while a sibling namespace called groupother stays out.
--username and --password could not have worked since 2022: get_client passed email= and password= to gitlab.Gitlab, which python-gitlab removed in 3.0, so every invocation died with "RequestsBackend.__init__() got an unexpected keyword argument 'email'". GitLab removed password authentication from its API, so there is nothing to map the flags onto. They now fail immediately with a pointer to --private-token, and are hidden from --help so the documented interface only lists what works. Keeping them registered means an existing script gets that message rather than an unrecognized-arguments error. The existing tests could not have caught this: they mock gitlab.Gitlab, and a mock accepts any keyword. Added a test that constructs the real client, so the next signature change in python-gitlab surfaces here.
Without git on PATH the run produced "[Errno 2] No such file or directory: 'git'", once per repository, naming neither the tool nor what to do about it. subprocess.call raises rather than returning a code when the binary is missing, which the git-lfs check did not allow for either. Both checks now handle that, and git is checked once at startup instead of failing 200 times over.
With neither --owned-only nor --with-membership, the project listing asked gitlab for everything the token could see, which includes every public project on the instance. Against gitlab.com that enumerates millions of projects at roughly 27 a second: the tool printed nothing and never reached the first clone, which is indistinguishable from a hang. --namespace did not help, being applied after the listing had finished. The default is now the projects the user is a member of, which is what a backup tool is expected to mean. The old behaviour is still available as --all-visible, useful on a small self-hosted instance, with a warning in its help text. The three scope flags are mutually exclusive and the chosen scope is logged, so a run is never ambiguous about what it is collecting.
--git-timeout bounded every git command but nothing bounded the api client, so projects.list against an unresponsive host still blocked forever. It now takes --api-timeout, 60 seconds by default. Also here, each small enough not to warrant its own commit: - --quiet set the module logger while --log-level set the root, so --quiet --log-level DEBUG silently produced no debug output. Both drive the same logger now and an unknown level is rejected rather than ignored. - main resolved output_directory but backup_repository used the raw value, so the directory created and the one cloned into came from different strings. - logging.basicConfig ran at import time, reconfiguring logging for anything that imported the module. - --private-key added alongside --private_key, which stays working. - --version, which the package had but never exposed.
release-requirements.txt listed 32 transitive dependencies alongside the ten tools actually used, so dependabot tracked packages nothing asks for directly. Trimmed to the direct ones, grouped by purpose, matching what python-github-backup settled on. The docker build installed that whole toolchain into the runtime venv, so the published image shipped pytest, black and twine. Installing only requirements.txt takes it from 169MB to 117MB. lint ran black without --check, so CI reformatted the checkout and always passed regardless of formatting. .env is git-ignored: a token file sitting next to a large staged changeset is one git add away from being published.
The documented usage had drifted: it advertised --oath-token, a flag renamed in 2020, and omitted --owned-only. Regenerated from --help, so it now lists the scope, timeout and retry flags and no longer shows the two authentication flags that cannot work. Requirements corrected to git 2.31+, which the credential handling needs.
Two cases with no coverage: a path occupied by something that was never a repository, and an lfs fetch that exits non-zero. The second is the one that matters, since the lfs call was the last git invocation still missing its timeout and retries and nothing would have caught that.
mhajder
marked this pull request as ready for review
August 16, 2026 17:49
Owner
|
Mind rebasing? I merged a dependabot PR before this and there is a merge conflict 😬 |
# Conflicts: # release-requirements.txt
Contributor
Author
|
Fixed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gitlab_backup/held only a version string while all of logic lived inbin/gitlab-backup, so nothing could be imported and nothing could be tested. This moves the logic into the package, adds a test suite, and fixes the bugs that writing those tests and running the tool against gitlab.com uncovered.Structured as 16 commits (I could have split the code into more but I didn't want PR to have so many commits), each one green on its own, so the fixes can be reviewed or reverted individually.
Some of the code was generated by LLMs, mostly tests. Everything was verified manually.