repo.references() fails to deduplicate packed/loose refs

[yuja]

Current behavior 😯

With the following setup, repo.references() emits both loose and packed refs.

git init -q

mkdir -p .git/refs/heads/a
echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904 > .git/refs/heads/a-
echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904 > .git/refs/heads/a/b
echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904 > .git/refs/heads/a0
git pack-refs --all

This generates the following packed-refs:

# pack-refs with: peeled fully-peeled sorted
4b825dc642cb6eb9a060e54bf8d69288fbee4904 refs/heads/a-
4b825dc642cb6eb9a060e54bf8d69288fbee4904 refs/heads/a/b
4b825dc642cb6eb9a060e54bf8d69288fbee4904 refs/heads/a0
EOF

Create loose refs:

mkdir -p .git/refs/heads/a
echo 0000000000000000000000000000000000000000 > .git/refs/heads/a-
echo 0000000000000000000000000000000000000000 > .git/refs/heads/a/b
echo 0000000000000000000000000000000000000000 > .git/refs/heads/a0

List all refs:

fn main() {
    let path = env::args().nth(1).unwrap();
    let repo = gix::open(path).unwrap();
    for git_ref in repo.references().unwrap().all().unwrap() {
        let git_ref = git_ref.unwrap();
        println!("{} {:?}", git_ref.name().as_bstr(), git_ref.target());
    }
}

refs/heads/a/b in packed-refs should be omitted:

refs/heads/a- Object(Sha1(0000000000000000000000000000000000000000))
refs/heads/a/b Object(Sha1(4b825dc642cb6eb9a060e54bf8d69288fbee4904))
refs/heads/a0 Object(Sha1(0000000000000000000000000000000000000000))
refs/heads/a/b Object(Sha1(0000000000000000000000000000000000000000))

I think this is regression caused by the fix for #1850. Maybe we'll need to sort file-system entries in the same way as tree::EntryRef?

Expected behavior 🤔

No response

Git behavior

No response

Steps to reproduce 🕹

No response

[Byron]

Thanks a lot for reporting and for figuring out a reproduction.

It's going to go into the test-suite, from where I'd hope to be able to develop a patch. I perfectly agree that it's related to sorting.

Since you went so far, I wouldn't want to be in your way of submitting a PR with a reproduction test-case, maybe you want to take a shot at figuring out the sorting as well?
It will probably take me a couple of days.

[yuja]

Sure, I'll take a look. Is it okay to add gix-features -> gix-path dependency? We'll probably need to convert OsStr to BStr.

[Byron]

Thank you! And absolutely, do what you must.

[ilyagr]

Meanwhile, with gix 0.71, I'm getting a similar problem to #1850 , but for remote references instead of local references this time. How do I check whether this is the same as the issue that Yuya found?

$ bat src/main.rs
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
File: src/main.rs
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
use std::path::Path;

use gix::bstr::{B, BString, ByteSlice};

fn main() -> anyhow::Result<()> {
    let repo = gix::discover(Path::new(
        "/Users/ilyagr/dev/jj",
    ))?;
    for b in repo.references()?.remote_branches()? {
        let b = b.unwrap();
        if b.name().as_bstr().find(b"squash-preserve").is_some() {
            dbg!(b);
        }
    }
    Ok(())
}
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
$ cargo run
[src/main.rs:12:13] b = Reference {
    name: FullName(
        "refs/remotes/upstream/ig/squash-preserve-snapshot",
    ),
    target: Object(
        Sha1(cbf363db4b7036e736660218edb099ce8dfda99c),
    ),
    peeled: None,
}
[src/main.rs:12:13] b = Reference {
    name: FullName(
        "refs/remotes/upstream/ig/squash-preserve-snapshot",
    ),
    target: Object(
        Sha1(389858b237528e259d9d81c1353fe5fd4dd5baa8),
    ),
    peeled: None,
}

The program that I copied above is almost identical to before:

diff --git a/src/main.rs b/src/main.rs
index 6a4aba9403..b67638212e 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,17 +1,14 @@
 use std::path::Path;

-use gix::{
-    bstr::{B, BString, ByteSlice},
-    date::time::format,
-    revision::walk::Sorting,
-};
+use gix::bstr::{B, BString, ByteSlice};

 fn main() -> anyhow::Result<()> {
-    let repo = gix::discover(Path::new("/Users/ilyagr/dev/jj"))?;
-    for b in repo.references()?.local_branches()? {
+    let repo = gix::discover(Path::new(
+        "/Users/ilyagr/dev/jj",
+    ))?;
+    for b in repo.references()?.remote_branches()? {
         let b = b.unwrap();
-        let pat: Vec<u8> = B("pr4021").into();
-        if b.name().as_bstr().find(pat.as_slice()).is_some() {
+        if b.name().as_bstr().find(b"squash-preserve").is_some() {
             dbg!(b);
         }
     }

[yuja]

Meanwhile, with gix 0.71, I'm getting a similar problem to #1850 , but for remote references instead of local references this time. How do I check whether this is the same as the issue that Yuya found?

Can you test with #1931 applied? The current git::import_refs() in jj is pretty much broken.

[ilyagr]

#1931 fixes it! Awesome! 🎉 🎉

BTW, unsurprisingly, running git gc or jj util gc --expire now is still a working workaround.

To be more precise, I did

diff --git a/Cargo.toml b/Cargo.toml
index 38f0489907..1f7c18f9cc 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,3 +7,5 @@
 anyhow = "1.0.96"
 gix = "0.71.0"

+[patch.crates-io]
+gix = { git = "https://github.com/yuja/gitoxide", branch = "push-klrqpplwxrkx" }

I guess the same would work for jj (for people who are willing to compile it from source).

[moroten]

I have tested with gix=0.68.0 and overriding with gix-features=0.41.1 and I still get refs reported that are not part of repo.references()?.prefixed(...) scope. The refs that are reported are loose refs which are not present in .git/packed-refs.

[Byron]

I don't know if that really works, gix should be at 0.71 to actually pick up the gix-features override.
However, if the issue still persists, please open a new issue with a reproduction.
Thank you.

[moroten]

Still the same problem with gix=0.71.0 overridden with gix-features=0.41.1 (1612c73).

[ilyagr]

It might help if @Byron could release gix 0.71.1 that depends on the new patch version.

I'm not sure this is a real issue, nor whether there's an easier solution, but we're thinking of making a patch release of jj for this bug. I'm worried that if jj only list the new gix-features release in its Cargo.lock (since it's an indirect dependency), it might not propagate to other people packaging jj or even cargo install jj-cli.

[Byron]

Cargo will auto-upgrade to the latest available gix-features upon installation and thus should pick up all released fixes. Feature flags aren't involved, and I don't think this fix can be suppressed or somehow not come through.
Maybe I am missing something though. I tested this, and it picks up [email protected] as one would expect.

I think if jj really wants to make sure, they could add gix-features as dummy-dependency in the latest version, and then create a patch release.
That dummy could be removed immediately after the release is done.

[ilyagr]

I think if jj really wants to make sure, they could add gix-features as dummy-dependency in the latest version, and then create a patch release.
That dummy could be removed immediately after the release is done.

That's good advice, thanks!