fix: make git ref resolution less greedy for HEAD

[abn]

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

[dimbleby]

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.

[neersighted]

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).

[abn]

@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

[dimbleby]

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.

[Kuba314]

Is there any plan to merge / fix this?

[abn]

@Secrus I have rebased this and fixed the mypy issue.

[github-actions[bot]]

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.