Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential breaking change in Repo.head() for annotated tags #1443

Closed
abn opened this issue Nov 18, 2024 · 3 comments
Closed

Potential breaking change in Repo.head() for annotated tags #1443

abn opened this issue Nov 18, 2024 · 3 comments

Comments

@abn
Copy link
Contributor

abn commented Nov 18, 2024

@jelmer following up from python-poetry/poetry#9849 (comment) and python-poetry/poetry#9748 (comment) raising this issue to triage if this is in-fact a dulwich issue or a usage issue on our end.

A simplified reproducer that matches steps we use.

reproducer.py

import tempfile
from pathlib import Path

from dulwich import porcelain
from dulwich.client import get_transport_and_path
from dulwich.refs import ANNOTATED_TAG_SUFFIX
from dulwich.repo import Repo


def main() -> None:
    with tempfile.TemporaryDirectory(delete=True) as tmpdir:
        target = Path(tmpdir).joinpath("test-fixture-vcs-repository").as_posix()
        url = "https://github.com/python-poetry/test-fixture-vcs-repository.git"
        tag = "refs/tags/v0.1.0".encode()
        repo = Repo.init(target, mkdir=True)
        porcelain.remote_add(repo, "origin", url)

        config = repo.get_config_stack()
        client, path = get_transport_and_path(url, config=config)
        remote_refs = client.fetch(
            path,
            repo,
            determine_wants=repo.object_store.determine_wants_all,
        )

        annotated = tag + ANNOTATED_TAG_SUFFIX
        ref = annotated if annotated in remote_refs.refs else remote_refs.symrefs[b"HEAD"]
        head = remote_refs.refs[ref]
        remote_refs.refs[ref] = remote_refs.refs[b"HEAD"] = head

        repo.refs[b"HEAD"] = remote_refs.refs[b"HEAD"]

        with repo:
            repo.reset_index()

        print(repo.head(), repo.get_peeled(b"HEAD"))


if __name__ == '__main__':
    main()

container-one-line-reproducer.sh

podman run --rm -i --entrypoint bash docker.io/python:3.13 <<EOF
set -xe
cat > main.py <<PY
import tempfile
from pathlib import Path

from dulwich import porcelain
from dulwich.client import get_transport_and_path
from dulwich.refs import ANNOTATED_TAG_SUFFIX
from dulwich.repo import Repo


def main() -> None:
    with tempfile.TemporaryDirectory(delete=True) as tmpdir:
        target = Path(tmpdir).joinpath("test-fixture-vcs-repository").as_posix()
        url = "https://github.com/python-poetry/test-fixture-vcs-repository.git"
        tag = "refs/tags/v0.1.0".encode()
        repo = Repo.init(target, mkdir=True)
        porcelain.remote_add(repo, "origin", url)

        config = repo.get_config_stack()
        client, path = get_transport_and_path(url, config=config)
        remote_refs = client.fetch(
            path,
            repo,
            determine_wants=repo.object_store.determine_wants_all,
        )

        annotated = tag + ANNOTATED_TAG_SUFFIX
        ref = annotated if annotated in remote_refs.refs else remote_refs.symrefs[b"HEAD"]
        head = remote_refs.refs[ref]
        remote_refs.refs[ref] = remote_refs.refs[b"HEAD"] = head

        repo.refs[b"HEAD"] = remote_refs.refs[b"HEAD"]

        with repo:
            repo.reset_index()

        print(repo.head(), repo.get_peeled(b"HEAD"))


if __name__ == '__main__':
    main()

PY

python -m pip install --root-user-action=ignore --disable-pip-version-check -q 'dulwich==0.22.1'
python main.py
python -m pip install --root-user-action=ignore --disable-pip-version-check -q 'dulwich==0.22.3'
python main.py
python -m pip install --root-user-action=ignore --disable-pip-version-check -q 'dulwich==0.22.4'
python main.py
python -m pip install --root-user-action=ignore --disable-pip-version-check -q 'dulwich==0.22.5'
python main.py
python -m pip install --root-user-action=ignore --disable-pip-version-check -q 'dulwich==0.22.6'
python main.py
EOF

From the output of the above shell command, you can see that the change happened between 0.22.1 and 0.22.3.

+ python -m pip install --root-user-action=ignore --disable-pip-version-check -q dulwich==0.22.1
+ python main.py
b'b6204750a763268e941cec1f05f8986b6c66913e' b'b6204750a763268e941cec1f05f8986b6c66913e'
+ python -m pip install --root-user-action=ignore --disable-pip-version-check -q dulwich==0.22.3
+ python main.py
b'0be1215c90fffe37c5629cf2764615e9d9530e67' b'0be1215c90fffe37c5629cf2764615e9d9530e67'
+ python -m pip install --root-user-action=ignore --disable-pip-version-check -q dulwich==0.22.4
+ python main.py
b'0be1215c90fffe37c5629cf2764615e9d9530e67' b'0be1215c90fffe37c5629cf2764615e9d9530e67'
+ python -m pip install --root-user-action=ignore --disable-pip-version-check -q dulwich==0.22.5
+ python main.py
b'0be1215c90fffe37c5629cf2764615e9d9530e67' b'0be1215c90fffe37c5629cf2764615e9d9530e67'
+ python -m pip install --root-user-action=ignore --disable-pip-version-check -q dulwich==0.22.6
+ python main.py
b'0be1215c90fffe37c5629cf2764615e9d9530e67' b'0be1215c90fffe37c5629cf2764615e9d9530e67'
@jelmer
Copy link
Owner

jelmer commented Nov 18, 2024

I don't think this is a regression, but rather a bug fix. The new behaviour is correct and consistent with C git; symrefs and unpeeled tags are meant to be preserved by a clone rather than be dereferenced.

Even before 0.22.2 you were meant to get this behaviour, but that might not have happened in all situations.

@abn
Copy link
Contributor Author

abn commented Nov 18, 2024

I agree that given that this change makes things consistent across tools, it is the right change.

However, even if it is a bug fix this is likely a breaking change from a user's perspective (this is also debatable). I guess it comes down to if you follow semver, if not then downstream users just need to adjust the version expectations and tighten the version constraints.

Edit: As the "api" iteself has not changed - it is technically still conformant to semver norms. So ... /shrug!

@jelmer
Copy link
Owner

jelmer commented Nov 18, 2024

Yeah, I agree that it is surprising - if I'd been aware of it before the release I would have added a note in NEWS.

However, with certain client implementations (e.g. LocalGitClient) you would already get the behaviour you're seeing with the newer versions. So it's a bit of a grey area.

Anyway, glad we've at least cleared up what the issue was here. Just to be clear, the intent is to follow semver. Closing this for now.

@jelmer jelmer closed this as completed Nov 18, 2024
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

No branches or pull requests

2 participants