rust-lang / rust-lang/git2-rs

Should `Reference` implement `PartialEq`/`Eq`?

Open
#851 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
2.1k
Forks
450
Avg merge
11m
Merged PRs (30d)
1

Description

Currently, Reference has an implementation for Eq (and PartialEq) that uses git_reference_cmp under the hood to determine if two references are "equal".

However, git_reference_cmp may return 0 ("yes they're equal") when comparing two symbolic references that have different names but the same target. See libgit2/libgit2#6337 for further details, and refs.c:1069 for the current implementation of git_reference_cmp, effectively comparing the targets of symbolic references instead of the references themselves.

This leads to the following behaviour:

# create the refs (they can also be created using git2, but using the git CLI here for simplicity)
git symbolic-ref refs/foo refs/heads/master
git symbolic-ref refs/bar refs/heads/master
let foo = repo.find_reference("refs/foo")?;
let bar = repo.find_reference("refs/bar")?;

assert_ne!(foo.name(), bar.name()); // ok, "refs/foo" != "refs/bar" (disregarding the `Option` here)
assert_ne!(foo, bar); // assertion fails! despite being *different* references, they share the same target

While it can make sense in some cases to have git_reference_cmp behave the way it does, I'm not sure if this is expected for an implementation of PartialEq, and especially so for an implementation of Eq. It's easy to overlook, especially since I couldn't find any mention of this in the documentation (of both git2-rs or libgit2).

Maybe git2-rs shouldn't translate git_reference_cmp to a PartialEq/Eq implementation? Maybe libgit2 should change the behaviour of git_reference_cmp? Maybe nothing should change and the documentation should just make this more clear? I'm not too sure what's best here, but this has definitely confused me more than once.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the Reference PartialEq/Eq implementation and libgit2's refs.c:1069 behavior, then compare the available documentation for reference equality. A complete resolution needs a decided equality contract, corresponding tests for differently named symbolic references, and documentation if the existing behavior remains.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.