Fix AssertionError when cloning at annotated tag#10719
Open
jelmer wants to merge 1 commit intopython-poetry:mainfrom
Open
Fix AssertionError when cloning at annotated tag#10719jelmer wants to merge 1 commit intopython-poetry:mainfrom
jelmer wants to merge 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's GuideEnsures Git clone operations at annotated (including nested) tags always set HEAD to the underlying commit object by peeling tag objects recursively, and adds regression tests for annotated and nested annotated tag cloning using real dulwich repos. Sequence diagram for resolving HEAD at annotated tagsequenceDiagram
actor Developer
participant GitBackend
participant FetchPackResult
participant Repo
participant ObjectStore
participant Tag
participant Commit
Developer->>GitBackend: resolve(remote_refs, repo)
GitBackend->>GitBackend: _normalise(remote_refs, repo)
GitBackend->>GitBackend: _set_head(remote_refs, repo)
alt ref_is_symbolic_HEAD
GitBackend->>FetchPackResult: read refs[ref]
FetchPackResult-->>GitBackend: head
else ref_is_specific_tag_or_ref
GitBackend->>FetchPackResult: read refs[ref]
FetchPackResult-->>GitBackend: head
end
GitBackend->>Repo: access object_store
Repo-->>GitBackend: ObjectStore
GitBackend->>ObjectStore: get(head)
alt object_found
ObjectStore-->>GitBackend: Tag or Commit
loop peel_tag_until_commit
GitBackend->>GitBackend: isinstance(obj, Tag)
alt obj_is_Tag
GitBackend->>Tag: read object
Tag-->>GitBackend: object_type, sha
GitBackend->>ObjectStore: get(sha)
ObjectStore-->>GitBackend: Tag or Commit
else obj_is_not_Tag
GitBackend->>GitBackend: obj is Commit (stop peeling)
end
end
else object_missing
ObjectStore-->>GitBackend: KeyError
GitBackend->>GitBackend: skip peeling (handled during fetch)
end
GitBackend->>FetchPackResult: set refs[ref] = head
GitBackend->>FetchPackResult: set refs[HEAD] = head
GitBackend-->>Developer: HEAD now points to Commit
Updated class diagram for Git backend tag peeling logicclassDiagram
class GitBackend {
- bytes ref
- str revision
+ resolve(remote_refs FetchPackResult, repo Repo) void
- _normalise(remote_refs FetchPackResult, repo Repo) void
- _set_head(remote_refs FetchPackResult, repo Repo) void
}
class FetchPackResult {
+ dict~bytes, bytes~ refs
}
class Repo {
+ ObjectStore object_store
}
class ObjectStore {
+ __getitem__(sha bytes) GitObject
}
class GitObject {
}
class Tag {
+ tuple~str, bytes~ object
}
class Commit {
}
GitBackend --> FetchPackResult : uses
GitBackend --> Repo : uses
Repo --> ObjectStore : has
ObjectStore <|.. GitObject : returns
GitObject <|-- Tag
GitObject <|-- Commit
GitBackend ..> Tag : peels
GitBackend ..> Commit : ensures_HEAD_points_to
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The tag-peeling loop in
_set_headhas no safeguard against cyclic tag references; consider tracking visited SHAs or imposing an iteration limit to avoid potential infinite loops on malformed repositories. - Swallowing
KeyErrorin the tag-peeling block leavesheadpotentially pointing at a tag object again; it may be safer to either log/raise in this case or ensure the object is fetched/available before proceeding so HEAD is guaranteed to resolve to a commit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tag-peeling loop in `_set_head` has no safeguard against cyclic tag references; consider tracking visited SHAs or imposing an iteration limit to avoid potential infinite loops on malformed repositories.
- Swallowing `KeyError` in the tag-peeling block leaves `head` potentially pointing at a tag object again; it may be safer to either log/raise in this case or ensure the object is fetched/available before proceeding so HEAD is guaranteed to resolve to a commit.
## Individual Comments
### Comment 1
<location> `tests/vcs/git/test_backend.py:295-304` </location>
<code_context>
+@pytest.mark.skip_git_mock
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the test by asserting that HEAD matches the expected commit SHA, not just that it is a Commit object.
In `test_clone_annotated_tag`, you currently only check that `HEAD` is a `Commit`, which guards against it being a `Tag`. To make the test stronger, also assert that this `Commit` is the one created in the source repo by capturing its SHA (e.g., from `porcelain.commit` or `repo.head()` before tagging) and comparing that value to `head_sha`. This verifies both the type and that the peeling logic resolves to the correct commit.
Suggested implementation:
```python
from dulwich import porcelain
from dulwich.objects import Commit
```
```python
# Create a source repository with an annotated tag
```
```python
repo = Repo.init(str(source_path))
```
```python
# HEAD should be a commit (not a Tag) and it should be the same commit
# that was created in the source repository before tagging.
assert isinstance(head, Commit)
assert head_sha == expected_head_sha
```
I only see the beginning of `test_clone_annotated_tag`, so you will need to wire in the expected SHA where the initial commit is created:
1. When you create the commit in the source repository (likely via `porcelain.commit` or `repo.do_commit`), capture its SHA, for example:
- If using `porcelain.commit(str(source_path), message=b"Initial commit")`, assign the return value to `expected_head_sha`.
- If using `repo.do_commit(...)`, use `expected_head_sha = repo.head()` (or `repo[repo.head()].id` depending on how you compute `head_sha`).
2. Ensure that `head_sha` in the assertion block refers to the SHA of `HEAD` in the cloned repo (whatever variable you currently derive from the peeled `HEAD` object).
3. Make sure `expected_head_sha` is defined in the same scope as the final assertions so that `assert head_sha == expected_head_sha` compiles and runs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
When cloning a Git repository at an annotated tag, if the peeled tag
reference (refs/tags/v1.0.0^{}) is not available in the fetch result,
Poetry would set HEAD to the tag object SHA instead of the commit SHA.
This caused reset_index() to fail with:
AssertionError: assert isinstance(obj, Commit)
The fix peels tag objects recursively to extract the underlying commit
SHA before setting HEAD. This ensures HEAD always points to a Commit
object, not a Tag object.
Fixes python-poetry#10658
jelmer
force-pushed
the
fix-peeled-tags
branch
from
1b7971a to
65d0c5f
Compare
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
1 participant
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.
When cloning a Git repository at an annotated tag, if the peeled tag reference (refs/tags/v1.0.0^{}) is not available in the fetch result, Poetry would set HEAD to the tag object SHA instead of the commit SHA. This caused reset_index() to fail with:
The fix peels tag objects recursively to extract the underlying commit SHA before setting HEAD. This ensures HEAD always points to a Commit object, not a Tag object.
Dulwich has already been updated to print clearer errors in this situation, which should be in 1.0.1
Pull Request Check List
Resolves: #10658