wordpress-mobile / wordpress-mobile/release-toolkit

`GitHelper.checkout_and_pull` swallows git errors and reports failure only via its return value

Open
#756 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement release-toolkit
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_pull rescues StandardError and returns true/false instead 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 checkout or git 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

  1. Add a raising variantcheckout_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.
  2. Or raise by default with an opt-outcheckout_and_pull(branch, fatal: true), defaulting fatal to true so the safe behavior is the default. (Breaking for any caller currently relying on the false return.)
  3. At minimum, document that the return value must be checked, and align the !-naming across GitHelper so 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.