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

[neersighted]

This looks a bit better than the first attempt (and has tests), but I am a bit wary of it still. Do you have any knowledge of where the duplicate clone (with extras) comes from/how we can prevent it?

[dimbleby]

We clone the package twice in spite of the cache on

poetry/src/poetry/puzzle/provider.py

Lines 88 to 96 in 25767bc

	@functools.lru_cache(maxsize=None)
	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:

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 #6874 (comment) 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

poetry/src/poetry/vcs/git/backend.py

Line 44 in 25767bc

class GitRefSpec:

[dimbleby]

cf python-poetry/poetry-plugin-export#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 #6874 (comment) 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.

[Secrus]

LGTM

[github-actions]

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.