Use gitoxide for get_tags
#2664
Conversation
cruessler
force-pushed
the
use-gitoxide-for-tags
branch
from
8604295 to
49cd6a7
Compare
cruessler
force-pushed
the
use-gitoxide-for-tags
branch
from
3e1c48f to
1750d96
Compare
cruessler
force-pushed
the
use-gitoxide-for-tags
branch
from
1750d96 to
7dcf8d8
Compare
|
@cruessler lgtm. do we need a @Byron review? do you know of anything not working as before? |
|
Based on my observations, the 
I haven’t found any issues yet, but I also haven’t been using the tag popup on a regular basis. If @Byron wants to have a quick last look, that would be welcome, but not strictly necessary. :-) |
| impl From<gix::Commit<'_>> for CommitId { | ||
| fn from(commit: gix::Commit<'_>) -> Self { | ||
| #[allow(clippy::expect_used)] | ||
| let oid = Oid::from_bytes(commit.id().as_bytes()).expect("`Oid::from_bytes(commit.id().as_bytes())` is expected to never fail"); |
There was a problem hiding this comment.
(Nit) I often struggle with good .expect("messages") as they shouldn't be redundant, or else I could use the .unwrap() I want to avoid. Here I'd probably write that git2 commits are always 20 bytes long as the reason I expect this to always work.
Funnily enough, while writing it I was wondering… are they? Is there SHA256 and would that work then? Answer: certainly once gix has SHA256 compiled in, but it felt interesting how a good .expect() makes you think.
|
|
||
| return true; | ||
| let gix_repo: gix::Repository = | ||
| gix::ThreadSafeRepository::discover_with_environment_overrides(repo_path.gitpath()) |
There was a problem hiding this comment.
When seeing this I think gix should have an open_with_environment_overrides. Even though discover won't actually get to go upwards, that would much more clearly indicate the intent.
A PR is very welcome, it should not be more than a refactor.. The key here is really just
if std::env::var_os("GIT_DIR").is_some() {
return Self::open_with_environment_overrides(directory.as_ref(), trust_map).map_err(Error::Open);
}which could also be part of the open_with_environment_overrides() that we really want here.
Pretty please :D?
| let commit = reference.peel_to_commit(); | ||
| let tag = reference.peel_to_tag(); |
There was a problem hiding this comment.
To be extra-awesome, I'd invert the logic here and peel_to_tag() only, and then get the object and peel that again, and only if it's not a tag, peel to a commit.
To streamline this, there is reference.peel_to_kind(gix::object::Kind::Tag), and if that fails, one can peel again to a Commit maybe. This method is mutating, so one won't lose progress - if it's not a tag it's a commit (usually), and it will do no work to realise that.
But if you want nice code, this is probably it, and I don't think this is a performance issue either.
Gosh, I shouldn't spend my time with premature performance optimisation!
This PR is a draft, currently intended for a discussion with @Byron about the part of the API to be used for getting a reference’s name. I intend to mark it as ready once I’ve figured out how to best use the existing APIs.
Update 2025-06-09: this PR is now ready for review.
This PR changes the following:
get_tagsto usegitoxideunder the hood.gitoxideversion properly show an@symbol in the tags popup for annotated tags, something thelibgitversion doesn’t do. (I didn’t investigate this in depth, though.)I followed the checklist:
make checkwithout errors