Skip to content

Refactor return code docs [1.4-maint]#9948

Merged
ThomasWaldmann merged 1 commit into
borgbackup:1.4-maintfrom
PhrozenByte:docs-return-codes-1.4
Jul 24, 2026
Merged

Refactor return code docs [1.4-maint]#9948
ThomasWaldmann merged 1 commit into
borgbackup:1.4-maintfrom
PhrozenByte:docs-return-codes-1.4

Conversation

@PhrozenByte

Copy link
Copy Markdown
Contributor

Manual backport of #9945 to match Borg 1 default behaviour

@ThomasWaldmann

Copy link
Copy Markdown
Member

review by claude opus 4.8

Summary

Docs-only, one file. Content is accurate and a clear improvement over what was there. I checked the prose against the actual 1.4-maint code and verified the exit-code behaviour empirically. Two things should be fixed before merge (one of them also affects master).

Verification of the warning-grouping claim

Borg returns the specific warning code (rc 100..127) if there were one or more warnings of the same kind.

This is correct. get_ec() in src/borg/helpers/__init__.py:

rcs = sorted(set(w_info.wc for w_info in _warnings_list))
if len(rcs) == 1:
    # Easy: there was only one kind of warning, so we can be specific.
    return rcs[0]
# There were different kinds of warnings.
return EXIT_WARNING

The set() collapses duplicates, so N warnings sharing one code give len(rcs) == 1 and the specific code is returned; only distinct codes fall through to rc 1. Master's get_ec() is identical, so the same wording in #9945 holds there too.

Checked against borg 1.4.5 with BORG_EXIT_CODES=modern:

case rc
4x BackupPermissionError (extract into read-only dir) 105
2x IncludePatternNeverMatchedWarning 101
mixed (105 + 101) 1

The rest of the prose also matches the code: group precedence signal > error > warning > success matches max_ec(); "the first signal or error determines the final return code" matches max_ec() returning ec1 when both are of the same class; SIGINT -> 130, SIGTERM -> 143, SIGHUP -> 129 logged at ERROR matches archiver.py main() plus do_show_rc(); the legacy/modern default matches modern_ec in helpers/errors.py.

Findings

1. Broken cross-reference (blocking)

Last line of the file:

For a list of all specific error and warning codes, see :ref:msgid.

The backticks are missing, so this is not a reST role at all: it renders as the literal text :ref:msgid. with no link, and Sphinx emits no warning about it. The line it replaces had it right (:ref:`msgid` ).

For a list of all specific error and warning codes, see :ref:`msgid`.

Note the same regression was merged to master by #9945 (c7a53b1), so a follow-up fix is needed there as well.

2. "see list below" is inaccurate

Both table rows say "see list below", but the message id list lives in docs/internals/frontends.rst (the .. _msgid: label), which is a different page from docs/usage/. Nothing is below. Suggest "see :ref:msgid" in the rows, or dropping the phrase. Same issue on master.

3. Minor: over-narrow scoping

The paragraph is prefixed "If modern exit codes are enabled (BORG_EXIT_CODES=modern):", but the grouping/precedence and first-error rules apply unconditionally; max_ec()/get_ec() run in legacy mode too, where all warnings simply carry wc=1 so the same-kind rule trivially yields rc 1. Only the specific codes are modern-gated, which the sentence already states. Reads fine, it just implies the whole mechanism only exists in modern mode.

Non-blocking notes on the underlying behaviour

Not defects in this PR, but relevant if you want the docs to be exact:

  • The specific-warning selection only runs while the global _exit_code is still EXIT_SUCCESS. Any set_ec(EXIT_WARNING) short-circuits get_ec() and forces rc 1 regardless of the warnings list; call sites on 1.4-maint are the two check failures in archiver.py (do_check) and the xattr restore warning in archive.py. So an extract with ten permission warnings plus one xattr warning returns 1, not 105. That is defensible under the "warnings of different kinds -> rc 1" reading (the generic warning is its own kind), so the text is not wrong, but the mechanism is a short-circuit rather than a merge.
  • A few warnings use wc=None (e.g. the chunker-params/diff-will-be-slow notes and two borg check --repair messages) and never influence the rc at all, which "All errors and warnings are still logged individually" does not hint at.

Verdict: LGTM once the :ref: backticks are fixed (here and in master), ideally with the "list below" wording too.

@PhrozenByte
PhrozenByte force-pushed the docs-return-codes-1.4 branch from e85665b to 19f3001 Compare July 24, 2026 10:22
@PhrozenByte

PhrozenByte commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author
  1. Broken cross-reference: Fixed 👍 Weird, any idea why the CI checks didn't pick that up with Refactor return code docs #9945? Follow-up for master here: Fix return code docs #9951
  2. "see list below" is inaccurate: Changed to "see below" (only because I had to touch it anyway; Claude's claim that "nothing is below" is simply wrong, the link to the list is below; having the same cross-reference twice right after each other would be clumsy, removing it altogether wouldn't be an improvement)
  3. over-narrow scoping: I disagree, this sentence merely exists because it establishes the difference between errors and warnings; with legacy codes there's just rc 1 and rc 2 anyway, so there's no "first rc within a group wins", so no change necessary here IMO

@ThomasWaldmann

ThomasWaldmann commented Jul 24, 2026

Copy link
Copy Markdown
Member

ad 1.: CI only renders the docs, but is not clever enough to see that this piece of "plaintext" should have been a rst reference.

ad 2.: ok

ad 3.: that "first signal / first error" is a bit strange: from borg's perspective of the fatal stuff, there can be only one - there can be no second signal/error because borg has terminated when the "first-and-only-one" hit. nitpicking. :-)

@ThomasWaldmann
ThomasWaldmann merged commit e58da96 into borgbackup:1.4-maint Jul 24, 2026
4 checks passed
@ThomasWaldmann

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants