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

Try finding commit using short SHA if it is not on HEAD #9748

Merged
merged 9 commits into from
Nov 18, 2024

Conversation

mikhainin
Copy link
Contributor

@mikhainin mikhainin commented Oct 10, 2024

Pull Request Check List

If a dependency specifies a git revision using a short hash, Dulwich does not provide an option to directly address it (see discussion in #9560); however, we can still list revisions and find what we are looking for.

Resolves: #9560 #6455

  • Added tests for changed code.
  • Updated documentation for changed code.

@mikhainin
Copy link
Contributor Author

mikhainin commented Oct 10, 2024

It seemed to work in my case, but I would appreciate it if someone looked into this and told me if it makes sense.
Happy to add tests - I didn't find them for this functionality but my guess it should be somewhere in tests/vcs/git?

I can see some integration tests against https://github.com/python-poetry/test-fixture-vcs-repository but every commit there seems to have its own head/tag which does not allow to reproduce the initial issue...

@mikhainin mikhainin force-pushed the fix-short-sha branch 2 times, most recently from 5db267b to a5b0b85 Compare October 10, 2024 11:08
@radoering
Copy link
Member

@jelmer I'd appreciate if you could take a look if it makes sense (from a dulwich point of view).

@radoering
Copy link
Member

Happy to add tests - I didn't find them for this functionality but my guess it should be somewhere in tests/vcs/git?

We have tests in tests/integration/test_utils_vcs_git.py and in main/tests/vcs/git/test_backend.py. Fast tests that do not require an external repository can be put into the latter.

I can see some integration tests against https://github.com/python-poetry/test-fixture-vcs-repository but every commit there seems to have its own head/tag which does not allow to reproduce the initial issue...

I have no idea if it is feasible, but maybe we can create a local repository in the test?

@jelmer
Copy link
Contributor

jelmer commented Oct 27, 2024

Creating a local repository in e.g. a tempdir should be fast and easy with Dulwich - Repo.init(path) will do what you need.

@mikhainin
Copy link
Contributor Author

mikhainin commented Oct 28, 2024

I added the test, I had to introduce a new marker "skip_git_mock" not to mock "git clone" as that one is not ready to mock local clone.

The rest was copied from tests/vcs/git/test_system.py: I moved fixtures into separate files so they can be re-used and added a "middle commit" so the issue was finally reproduced.

@jelmer
Copy link
Contributor

jelmer commented Oct 28, 2024

I added the test, I had to introduce a new marker "skip_git_mock" not to mock "git clone" as that one is not ready to mock local clone.

The rest was copied from tests/vcs/git/test_system.py: I moved fixtures into separate files so they can be re-used and added a "middle commit" so the issue was finally reproduced.

LGTM, for what it's worth.

pyproject.toml Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/vcs/git/test_backend.py Outdated Show resolved Hide resolved
tests/vcs/git/test_backend.py Outdated Show resolved Hide resolved
tests/vcs/git/git_fixture.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
src/poetry/vcs/git/backend.py Outdated Show resolved Hide resolved
src/poetry/vcs/git/backend.py Outdated Show resolved Hide resolved
@abn
Copy link
Member

abn commented Nov 16, 2024

This will likely also require #9849 if we intend to update dulwich.

@dimbleby
Copy link
Contributor

dimbleby commented Nov 17, 2024

This will likely also require #9849 if we intend to update dulwich.

Since recent dulwich releases are bumping only the patch number, poetry users are already getting the latest dulwich.

Are you saying that dulwich has released a breaking change in a patch release?

Edit the poetry 1.8 branch is still on the dulwich 0.21 series, so I am wrong about poetry users already getting the latest.

Nevertheless #9849 says it was fixing something from "dulwich >=0.22.2" so the question stands

@jelmer
Copy link
Contributor

jelmer commented Nov 17, 2024

I'm curious what change is, we haven't had reports of API breakages.

abn
abn previously approved these changes Nov 18, 2024
@abn
Copy link
Member

abn commented Nov 18, 2024

@jelmer To close the loop regarding @dimbleby 's question; I have raised a dulwich issue for triage with a reproducer.

Cross-post: jelmer/dulwich#1443

@radoering radoering enabled auto-merge (squash) November 18, 2024 16:21
@radoering radoering merged commit d09f238 into python-poetry:main Nov 18, 2024
73 checks passed
@mikhainin mikhainin deleted the fix-short-sha branch November 18, 2024 21:18
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.

Git-Dependency with non-HEAD rev fails to lock
6 participants