Skip to content

fix(git): apply timeout and fatal-error wrapping consistently (audit #14.1) - #267

Merged
sbalabanov merged 2 commits into
mainfrom
audit/fix-14-1-git-error-contracts
Aug 5, 2026
Merged

fix(git): apply timeout and fatal-error wrapping consistently (audit #14.1)#267
sbalabanov merged 2 commits into
mainfrom
audit/fix-14-1-git-error-contracts

Conversation

@sbalabanov

@sbalabanov sbalabanov commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Audit #14.1. Apply the established timeout and error-wrapping contract consistently to the remaining read-only Git operations.

Intent

Ensure fatal Git failures and timeouts remain classifiable by callers without changing IsAncestor's conditional exit-code-1 behavior.

Changes

  • Apply _gitTimeout and wrapError to RevParse, IsAncestor, GetCommitTimeSecond, and FileHashes.
  • Preserve (false, nil) for a valid non-ancestor result.
  • Strengthen the existing error-propagation assertions without adding broad shared method-level tests.

Test Plan

  • make gazelle
  • ./tools/bazel test //core/git:git_test --test_output=errors --test_env=GIT_CONFIG_COUNT=1 --test_env=GIT_CONFIG_KEY_0=commit.gpgSign --test_env=GIT_CONFIG_VALUE_0=false

Revert Plan

Revert this PR to restore the previous per-method Git error behavior.

Jira Issues

None.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Comment thread core/git/git_test.go Outdated
}
}

func TestReadMethods_FatalExitCodeWrapsErrFatal(t *testing.T) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the requested test in 349da11. I also removed the shared method-level test scaffolding that was only used by the three deleted tests, avoiding dead helpers while retaining the existing focused error-propagation assertions.

[addressed by agent]

Comment thread core/git/git_test.go Outdated
}
}

func TestReadMethods_WrapContextErrors(t *testing.T) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the requested test in 349da11. I also removed the shared method-level test scaffolding that was only used by the three deleted tests, avoiding dead helpers while retaining the existing focused error-propagation assertions.

[addressed by agent]

Comment thread core/git/git_test.go Outdated
}
}

func TestIsAncestor_ExitCodeOneMeansNotAncestor(t *testing.T) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the requested test in 349da11. I also removed the shared method-level test scaffolding that was only used by the three deleted tests, avoiding dead helpers while retaining the existing focused error-propagation assertions.

[addressed by agent]

.1)

Summary:
Audit #14.1 extracts the git command contract fixes from #262.

Intent:
- Ensure read-only git commands honor the same timeout and error-classification contract as mutating commands.
- Preserve conditional exit semantics for merge-base ancestry checks.

Changes:
- Apply the shared timeout and wrapError path to RevParse, IsAncestor, GetCommitTimeSecond, and FileHashes.
- Cover fatal exits, deadline expiration, parent cancellation, and IsAncestor exit code 1.
@sbalabanov
sbalabanov force-pushed the audit/fix-14-1-git-error-contracts branch from fe0c041 to 349da11 Compare August 3, 2026 23:50
@sbalabanov
sbalabanov marked this pull request as ready for review August 3, 2026 23:50
@sbalabanov
sbalabanov requested review from a team as code owners August 3, 2026 23:50
@sbalabanov
sbalabanov merged commit 9983def into main Aug 5, 2026
16 of 17 checks passed
@sbalabanov
sbalabanov deleted the audit/fix-14-1-git-error-contracts branch August 5, 2026 00:04
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.

4 participants