poetry
poetry copied to clipboard
fix: make git ref resolution less greedy for HEAD
This change ensures that when ref HEAD
is specified, it is not incorrectly resolved to refs/head/HEAD
. HEAD
is not treated as a branch, and correctly resolved from the ref spec.
Resolves: #7024 Closes: #6874
We clone the package twice in spite of the cache on https://github.com/python-poetry/poetry/blob/25767bcb4022a4a75e9c0f7c0eae5c8b8a72e12c/src/poetry/puzzle/provider.py#L88-L96 because the first time round rev
is None
and the second time it is "HEAD"
.
From that point of view the alternative fix that I tried in https://github.com/python-poetry/poetry/pull/6874#issue-1419899629 does better, but perhaps it's the wrong direction or has other disadvantages.
Right, that makes more sense. What do we think about introducing a new datatype here to avoid the overloaded strings and to encapsulate the HEAD == None
logic? The tests look good; we just need to realize that None
and HEAD
are the same thing (and to avoid the entropy of a string conversion from HEAD
causing the cache to miss and clone a second time).
@neersighted I kind of feel that adding a new datatype here might be overkill at present, but happy to be told otherwise. Alternatively, we could do something like this.
cache-fix.patch
diff --git a/src/poetry/puzzle/provider.py b/src/poetry/puzzle/provider.py
index 4d2e2d30..97abfb8c 100644
--- a/src/poetry/puzzle/provider.py
+++ b/src/poetry/puzzle/provider.py
@@ -86,7 +86,7 @@ class Indicator(ProgressIndicator): # type: ignore[misc]
@functools.lru_cache(maxsize=None)
-def _get_package_from_git(
+def _get_package_from_git_cached(
url: str,
branch: str | None = None,
tag: str | None = None,
@@ -118,6 +118,24 @@ def _get_package_from_git(
return package
+def _get_package_from_git(
+ url: str,
+ branch: str | None = None,
+ tag: str | None = None,
+ rev: str | None = None,
+ subdirectory: str | None = None,
+ source_root: Path | None = None,
+) -> Package:
+ return _get_package_from_git_cached(
+ url,
+ branch if branch != "HEAD" else None,
+ tag,
+ rev if rev != "HEAD" else None,
+ subdirectory,
+ source_root,
+ )
+
+
class Provider:
UNSAFE_PACKAGES: set[str] = set()
Either way, I can raise another PR to deal with the double clone issue. Let's solve the functionality issue by merging this?
Edit: Also there already is https://github.com/python-poetry/poetry/blob/25767bcb4022a4a75e9c0f7c0eae5c8b8a72e12c/src/poetry/vcs/git/backend.py#L44
cf https://github.com/python-poetry/poetry-plugin-export/issues/172 in which the HEAD
reference is again escaping where it oughtn't.
I wonder whether HEAD
simply isn't a good reference to use and this fix is just papering over that (and requiring more of the same elsewhere).
Perhaps the alternative at https://github.com/python-poetry/poetry/pull/6874#issue-1419899629 is better after all - though I seem to recall there was more unit test noise to work through in that case.
Is there any plan to merge / fix this?
@Secrus I have rebased this and fixed the mypy issue.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.