Visitar URL original
fix: validate object types when loading binary SHAs by Byron · Pull Request #2256 · gitpython-developers/GitPython · GitHub
Skip to content

fix: validate object types when loading binary SHAs - #2256

Merged
Byron merged 1 commit into
mainfrom
better-binsha-checks
Sep 28, 2026
Merged

fix: validate object types when loading binary SHAs#2256
Byron merged 1 commit into
mainfrom
better-binsha-checks

Conversation

@Byron

@Byron Byron commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Tasks

This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.

  • refackiew

Everything below this line was generated by Codex GPT-6.

Created by Codex on behalf of Byron. Byron will review before this is ready to merge.

Fixes #2254.

Commit(repo, tag.binsha).tree now raises a ValueError identifying the SHA, its actual type, and the expected type before parsing tag data as a commit. The shared check also covers trees, tags, blobs, object size/streams, and raw tree traversal. Validation happens on first data access, preserving lazy constructors and placeholder objects. Rejected streams are drained so subsequent git cat-file reads remain usable even while an exception retains its traceback.

The related audit found two more cases: IndexFile.new() converted binary SHAs to their Python string representation, and submodule diffs wrapped commit SHAs in Blob objects. Index creation now accepts binary SHAs and hexadecimal bytes alongside strings and trees; submodule diffs use IndexObject entries while retaining access to the submodule's commit data.

Existing object, index, and submodule diff tests were extended in place, including mixed binary/hex/tree arguments. No new test functions were added.

Validation:

  • 227 affected tests passed, with 2 skips, across the suite run and the staged-file test's rerun with init.defaultBranch=master. Commit signing was disabled only for test commands.
  • The extended object and index tests failed before the fix. The issue reproduction and valid binary-SHA operations were also checked with both GitCmdObjectDB and GitDB.
  • Ruff lint/format checks, mypy (46 source files), and basedpyright passed. The now-obsolete index assignment diagnostic was removed from the type-checking baseline.
  • Codex review of ca55dbf found no actionable regressions.

Git behavior reference: d38352cd43ab9745686d697872408bc3249a153f in the local Git checkout; the commit, tree, and tag readers check object types before parsing their contents.

@Byron
Byron force-pushed the better-binsha-checks branch from ca55dbf to 0610404 Compare September 28, 2026 07:41
@Byron
Byron marked this pull request as ready for review September 28, 2026 07:41
Copilot AI lite review requested due to automatic review settings September 28, 2026 07:41
Copilot AI reviewed Sep 28, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved stream-safety, type-normalization, and added-submodule diff issues require fixes.

Review effort: Lite
Findings: 4 High severity · 1 Medium severity

Open (5)
What changed in this PR

Adds lazy Git object-type validation, binary SHA support for index creation, and improved submodule diff handling.

Changes:

  • Validates object types before parsing or streaming.
  • Accepts binary and hexadecimal SHAs in IndexFile.new().
  • Uses IndexObject for submodule diff entries and extends tests.
File Summary
test/​test_index.py Tests binary SHA and type handling.
test/​test_diff.py Tests submodule diff behavior.
test/​test_commit.py Updates commit stream tests.
test/​test_base.py Tests object validation.
git/​objects/​util.py Implements shared type checking.
git/​objects/​tree.py Validates tree loading.
git/​objects/​tag.py Validates tag loading.
git/​objects/​fun.py Validates tree traversal.
git/​objects/​commit.py Validates commit loading.
git/​objects/​base.py Adds lazy object validation.
git/​index/​base.py Supports binary SHA inputs.
git/​diff.py Handles submodule commits as index objects.
.basedpyright/​baseline.json Removes an obsolete diagnostic.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread git/objects/base.py Outdated
Comment thread git/objects/fun.py Outdated
Comment thread git/objects/util.py Outdated
Comment thread git/objects/util.py Outdated
Comment thread git/diff.py
Copilot AI review requested due to automatic review settings September 28, 2026 11:24
@Byron
Byron force-pushed the better-binsha-checks branch from 0610404 to 7ed5433 Compare September 28, 2026 11:24
Copilot AI reviewed Sep 28, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved object-type, raw tree traversal, and submodule repository-selection issues block approval.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (5)

Comment thread git/objects/base.py Outdated
@Byron @codex
fix: validate size lookups and preserve binary SHAs (#2254)
6bf77c0 
<!-- agent -->
An object constructed with a SHA for another Git object type could
silently expose that object's size. Check the type metadata already
returned by `odb.info()` when loading an uncached `Object.size`, and
raise `ValueError` naming the SHA, actual type, and expected type.

Limit validation to this metadata lookup. Rejecting an unread stream can
leave payload bytes in the persistent `git cat-file --batch` response
while an exception retains its traceback, corrupting subsequent reads.
Stream consumers keep their existing behavior, with no type assertions
or added draining. Constructors remain lazy, and object-data parsing
still relies on callers to supply a SHA of the appropriate type.

Preserve binary SHAs in `IndexFile.new()` instead of converting them to
their Python string representation. Accept binary and hexadecimal bytes
alongside hex strings and `Tree` objects. Represent submodule diff
entries with `IndexObject`, retaining their path, mode, and access to the
submodule's commit data without labeling those commits as blobs.

Extend existing object, index, and submodule diff tests to cover binary
SHA inputs, metadata mismatches, and raw streaming through wrappers of
another object type. Remove the obsolete `IndexFile.new()` assignment
diagnostic from the `basedpyright` baseline.

Validation: 67 affected tests and Ruff lint/format checks passed.
Subsequent reads were also verified with active parser exceptions using
both `GitCmdObjectDB` and `GitDB`.

Assisted-by: GPT 6.0
Co-authored-by: GPT 6.0 <codex@openai.com>
Copilot AI review requested due to automatic review settings September 28, 2026 11:48
@Byron
Byron force-pushed the better-binsha-checks branch from 7ed5433 to 6bf77c0 Compare September 28, 2026 11:48
Copilot AI reviewed Sep 28, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Three moderate unresolved issues remain in object validation, tree traversal, and added gitlink handling.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
Resolved since last review (1)

Comment thread git/index/base.py
Comment thread git/objects/base.py
@Byron
Byron merged commit 3c638d2 into main Sep 28, 2026
51 checks passed
@Byron
Byron deleted the better-binsha-checks branch October 5, 2026 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Exception suggestion: raise when making a Commit with non-commit binsha

2 participants

@Byron