wordpress-mobile / wordpress-mobile/release-toolkit
`GitHelper.checkout_and_pull` swallows git errors and reports failure only via its return value
Nobody has claimed this yet.
- Dominant language
- Ruby
- Stars
- 31
- Forks
- 10
- Avg merge
- 5h 45m
- Merged PRs (30d)
- 2
Description
Fastlane::Helper::GitHelper.checkout_and_pull swallows all git errors and reports failure only through its return value, so the easy way to call it — ignoring the return — is also the unsafe way.
Summary
checkout_and_pullrescuesStandardErrorand returnstrue/falseinstead of raising.- A caller that ignores the return (the natural, and by far the most common, usage across our repos) silently proceeds after a failed
git checkoutorgit pull. - The failure behavior is inconsistent with its sibling helpers and can't be inferred from the method name (no
!), so callers don't realize they need to guard it.
def self.checkout_and_pull(branch)
branch = branch.first.join('/') if branch.is_a?(Hash)
Action.sh('git', 'checkout', branch)
Action.sh('git', 'pull')
true
rescue StandardError
false
end
The inconsistency
GitHelper has no consistent convention for how git failures surface, and the Ruby ! convention is arguably inverted here — the raising helper carries a bang, the silently-swallowing one doesn't:
| Method | On git failure | Bang? |
|---|---|---|
checkout_and_pull |
swallows → returns false |
no |
create_branch |
raises (no rescue) | no |
commit |
swallows → returns false |
no |
delete_local_branch_if_exists! |
raises (ruby-git) | yes |
How this would have helped us
In WordPress-Android#23001 we added a scheduled lane that resets a rolling branch to trunk, re-downloads translations, and opens a single rolling PR. Its first line is:
Fastlane::Helper::GitHelper.checkout_and_pull(DEFAULT_BRANCH)
The job runs on a persistent mac-metal agent with a reused checkout. If the git pull here fails — a network blip, or a stale/diverged local trunk left by a prior run — the error is swallowed and the lane proceeds to cut the sync branch from a stale trunk, then force-pushes and opens/refreshes a PR against the wrong base. Nothing aborts; the failure is invisible until someone notices the PR is behind.
Because the toolkit offers no raising variant, we had to re-implement the guard at the call site:
unless Fastlane::Helper::GitHelper.checkout_and_pull(DEFAULT_BRANCH)
UI.user_error!("Could not check out and pull #{DEFAULT_BRANCH}; aborting translation sync.")
end
Every caller that wants the safe behavior has to write this same boilerplate. A raising default (or an obvious raising variant) would have made the safe path the default one.
Proposed fix
- Add a raising variant —
checkout_and_pull!that raises on failure, keeping the bool version for the (rarer) callers that genuinely want to branch on the result. This matches Ruby's!convention and is non-breaking. - Or raise by default with an opt-out —
checkout_and_pull(branch, fatal: true), defaultingfataltotrueso the safe behavior is the default. (Breaking for any caller currently relying on thefalsereturn.) - At minimum, document that the return value must be checked, and align the
!-naming acrossGitHelperso failure behavior is inferable from the signature.
What we're not asking for
This isn't a blanket "make every helper raise." commit returning a bool is genuinely useful — "nothing to commit" is a common, valid no-op that callers want to sail past (the same WordPress-Android lane relies on exactly that for its prune step). The point is narrower: a failed checkout_and_pull is almost never something a caller wants to continue past, so for this helper the safe path should be the default rather than opt-in boilerplate.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with Fastlane::Helper::GitHelper.checkout_and_pull and compare its failure behavior with create_branch, commit, and delete_local_branch_if_exists!. Review the documented callers and decide which proposed failure contract is appropriate; done means checkout or pull failures are no longer silently ignored while callers that need a boolean result remain supported.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- git, ruby
- Domain
- devops, release
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100