Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
Release 0.14.1 (unreleased)
===========================

* Fix ``dfetch import`` mangling the namespace of a generic VCS URL whose path contains ``.git`` other than as a suffix
* Fix ``Version`` comparison raising ``AttributeError`` when compared against a non-``Version`` object
* Fix ``dfetch add`` matching a remote whose base URL is only a string prefix (not a path prefix) of the project URL
* Fix git ref resolution spuriously matching the first reference for an empty revision
* Strip the trailing newline from the git origin URL returned by ``get_remote_url``

Release 0.14.0 (released 2026-06-14)
===========================

Expand Down
2 changes: 1 addition & 1 deletion dfetch/manifest/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,7 @@ def find_remote_for_url(self, remote_url: str) -> Remote | None:
target = remote_url.rstrip("/")
for remote in self.remotes:
remote_base = remote.url.rstrip("/")
if target.startswith(remote_base):
if target == remote_base or target.startswith(remote_base + "/"):
return remote
return None

Expand Down
6 changes: 5 additions & 1 deletion dfetch/manifest/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,18 @@ class Version(NamedTuple):

def __eq__(self, other: Any) -> bool:
"""Check if two versions can be considered as equal."""
if not other:
if not isinstance(other, Version):
return False

if self.tag or other.tag:
return bool(self.tag == other.tag)

return bool(self.branch == other.branch and self.revision == other.revision)
Comment on lines 22 to 25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read-only verification: confirm equality/hash logic sites.
rg -n "def __eq__|if self.tag or other.tag|def __hash__|tuple.__hash__" dfetch/manifest/version.py

Repository: dfetch-org/dfetch

Length of output: 216


🏁 Script executed:

cat -n dfetch/manifest/version.py

Repository: dfetch-org/dfetch

Length of output: 1672


Fix __hash__ to match __eq__ semantics.

The __eq__ method (lines 22-25) treats tag as dominant: if either object has a tag, only tags are compared. However, __hash__ (line 29) hashes all three tuple fields. This breaks the hash/equality contract—two equal versions can hash differently, causing dict/set lookup failures.

For example, Version(tag="1.0", branch="", revision="") equals Version(tag="1.0", branch="main", revision="abc123") but produces different hashes.

Update __hash__ to hash only the fields that determine equality:

Suggested patch
     def __hash__(self) -> int:
-        """Hash based on the underlying tuple value."""
-        return tuple.__hash__(self)
+        """Hash consistent with Version equality semantics."""
+        if self.tag:
+            return hash(("tag", self.tag))
+        return hash(("branch_revision", self.branch, self.revision))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dfetch/manifest/version.py` around lines 22 - 25, The `__hash__` method does
not match the semantics of the `__eq__` method in the Version class. The
`__eq__` method prioritizes tag comparison—if either version has a tag, only
tags are compared, ignoring branch and revision. However, `__hash__` currently
hashes all three fields unconditionally. This violates Python's hash/equality
contract where equal objects must have equal hashes. Update the `__hash__`
method to hash only the fields that determine equality: if the tag exists, hash
only the tag; otherwise, hash the tuple of branch and revision. This ensures two
versions that are equal under `__eq__` will always produce the same hash value.


def __hash__(self) -> int:
"""Hash based on the underlying tuple value."""
return tuple.__hash__(self)

@property
def field(self) -> tuple[str, str]:
"""Return ``(kind, value)`` for the active field: tag, revision, or branch."""
Expand Down
2 changes: 1 addition & 1 deletion dfetch/util/purl.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def _vcs_namespace_and_name(remote_url: str) -> tuple[str, str, str]:
remote_url = f"ssh://{parsed.path.replace(':', '/')}"
else:
namespace, name = _namespace_and_name_from_domain_and_path(
remote_url, path.replace(".git", "")
remote_url, path.removesuffix(".git")
)
return namespace, name, remote_url

Expand Down
4 changes: 3 additions & 1 deletion dfetch/vcs/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@ def _find_branch_tip_or_tag_from_sha(
) -> tuple[str, str]:
"""Check all branch tips and tags and see if the sha is one of them."""
branch, tag = "", ""
if not rev:
return (branch, tag)
for reference, sha in info.items():
if sha.startswith(rev): # Also allow for shorter SHA's
if reference.startswith("refs/tags/"):
Expand Down Expand Up @@ -680,7 +682,7 @@ def get_remote_url() -> str:
"""Get the url of the remote origin."""
try:
result = run_on_cmdline(logger, ["git", "remote", "get-url", "origin"])
decoded_result = str(result.stdout.decode())
decoded_result = str(result.stdout.decode()).strip()
except SubprocessCommandError:
decoded_result = ""

Expand Down
20 changes: 20 additions & 0 deletions tests/test_add.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,26 @@ def test_determine_remote_returns_none_for_empty_remotes():
assert result is None


def test_determine_remote_requires_path_boundary():
"""An org-scoped remote must not match a different org sharing its prefix."""
m = Mock()
m.remotes = [_make_remote("myorg", "https://github.com/myorg")]
result = Manifest.find_remote_for_url(
m, "https://github.com/myorg-private/repo.git"
)
assert result is None


def test_determine_remote_matches_exact_and_subpath():
"""The boundary check still matches the remote itself and any URL beneath it."""
m = Mock()
m.remotes = [_make_remote("myorg", "https://github.com/myorg")]
assert Manifest.find_remote_for_url(m, "https://github.com/myorg") is not None
assert (
Manifest.find_remote_for_url(m, "https://github.com/myorg/repo.git") is not None
)


# ---------------------------------------------------------------------------
# Add command – non-interactive
# ---------------------------------------------------------------------------
Expand Down
16 changes: 16 additions & 0 deletions tests/test_git_vcs.py
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,22 @@ def test_ls_remote():
assert info == expected


def test_get_remote_url_strips_trailing_newline():
"""git remote get-url appends a newline that must not leak into the URL."""
with patch("dfetch.vcs.git.run_on_cmdline") as run_on_cmdline_mock:
run_on_cmdline_mock.return_value.stdout = b"https://github.com/org/repo.git\n"
assert GitLocalRepo.get_remote_url() == "https://github.com/org/repo.git"


def test_find_branch_tip_or_tag_from_sha_empty_rev_matches_nothing():
"""An empty revision must not spuriously match the first reference."""
info = {
"refs/heads/master": "33d11e10699bae03ba2a58a280e92494f4fa0d82",
"refs/tags/v1.0": "0e3b216c7ab365b67765e94aeb45085c4db029e0",
}
assert GitRemote._find_branch_tip_or_tag_from_sha(info, "") == ("", "")


@pytest.mark.parametrize(
"name, env_ssh, git_config_ssh, expected",
[
Expand Down
19 changes: 19 additions & 0 deletions tests/test_project_version.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,22 @@
def test_version_comparison(name, version_1, version_2, expected_equality):
actual_equality = version_1 == version_2
assert actual_equality == expected_equality


@pytest.mark.parametrize(
"other",
[
("", "main", "123"), # a plain tuple
"main", # a string
42, # an int
],
)
def test_version_comparison_with_non_version(other):
"""Comparing a Version with a non-Version must not crash."""
assert (Version(branch="main", revision="123") == other) is False


def test_version_remains_hashable():
"""Defining __eq__ must not break hashing/set membership."""
version = Version(tag="1.0")
assert version in {version}
Comment on lines +220 to +223

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Strengthen the hashability test to cover equal-but-distinct instances.

The current assertion only proves an object is hashable with itself. It won’t catch __eq__/__hash__ contract mismatches for two different Version objects that compare equal.

Suggested test extension
 def test_version_remains_hashable():
     """Defining __eq__ must not break hashing/set membership."""
     version = Version(tag="1.0")
     assert version in {version}
+
+    left = Version(tag="1.0", branch="main", revision="abc")
+    right = Version(tag="1.0", branch="develop", revision="xyz")
+    assert left == right
+    assert hash(left) == hash(right)
+    assert right in {left}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_project_version.py` around lines 220 - 223, The
test_version_remains_hashable function only verifies that a single Version
instance can be in a set, but doesn't validate the critical __eq__/__hash__
contract for equal-but-distinct instances. Strengthen the test by creating two
separate Version objects with identical tag values (both "1.0"), asserting they
compare as equal, verifying they have the same hash value, and confirming that a
set containing one Version instance will correctly identify the other equal
Version instance as a member (using the 'in' operator). This will catch
violations of Python's rule that equal objects must have equal hashes.

5 changes: 5 additions & 0 deletions tests/test_purl.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,11 @@
"git://git.git.savannah.gnu.org/automake.git",
"pkg:generic/automake?vcs_url=git://git.git.savannah.gnu.org/automake.git",
),
# A ".git" substring inside the path must not be stripped (only the suffix)
(
"https://gitlab.com/group/foo.github/project.git",
"pkg:generic/group/foo.github/project?vcs_url=https://gitlab.com/group/foo.github/project.git",
),
# Trailing slash – issue #1137
("https://github.com/cpputest/cpputest/", "pkg:github/cpputest/cpputest"),
("https://github.com/dfetch-org/dfetch/", "pkg:github/dfetch-org/dfetch"),
Expand Down
Loading