fix: _vlog returns 0 when verbose=false (regression in install rc)#13
Merged
Conversation
The 'is_verbose && _log' chain short-circuits with rc=1 when verbose is off. _vlog was the last statement in download_url, so the entire function returned 1 on a successful download. install() then counted it as a failure, so install_from also returned non-zero on every real install when verbose=false (the default). Unit tests didn't catch this: no test asserted download_url returns 0 on a real successful download (they were either snapshot-based, mock the curl call, or only check filesystem state). Fix: - _vlog now uses an if-block and explicit 'return 0' at the end. - 7 new tests assert non-zero RC on every relevant path: download_url returns 0 on success (default + verbose-on + verbose-off forms), install returns 0, install_from returns 0, _vlog returns 0 in both modes.
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
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.
Summary
Senior-QA acceptance run against 0.4.1 (with the README quick-start layout) caught a deeper bug than the clean/doctor self-exclusion fix:
`bashdep::_vlog` ended with the chain `bashdep::is_verbose && bashdep::_log "$@"`. When verbose is off (the default), `is_verbose` returns 1, the chain short-circuits with rc=1, and that becomes the function's exit code. `_vlog` is the last statement of `bashdep::download_url` (` lockfile: $lock_file` line, added with the verbose-mode commit), so `download_url` returned 1 on every successful download.
`bashdep::install` then counted each successful install as a failure:
```bash
bashdep::download_url ... || failures=$((failures + 1))
```
So `install` returned the number of "failures" (= number of installed deps) and `install_from` propagated it. Real-world impact:
```bash
source lib/bashdep
bashdep::install_from .bashdep || exit $? # exits 1 on a perfectly successful install
```
Why unit tests missed it
No test asserted `download_url` returns 0 on a real successful download:
The bug only fired when verbose was off (default) AND a real download path ran end-to-end. Mocked download_url tests never exercised `_vlog`.
Fix
```bash
function bashdep::_vlog() {
if bashdep::is_verbose; then bashdep::_log "$@"; fi
return 0
}
```
Always return 0 — "not verbose" is a normal state, not a failure.
New tests (7)
Test plan