Skip to content

Use gitoxide for get_tags #2664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 6, 2025
Merged

Use gitoxide for get_tags #2664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1ff41d
Use gitoxide in get_tags
cruessler Jun 9, 2025
5af9167
Use reference.name().shorten()
cruessler Jun 9, 2025
7dcf8d8
Call gix::Tag::decode only once
cruessler Jun 9, 2025
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
22 changes: 21 additions & 1 deletion asyncgit/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub enum GixError {

///
#[error("gix::object::find::existing::with_conversion::Error error: {0}")]
ObjectFindExistingWithConversionError(
ObjectFindExistingWithConversion(
#[from] gix::object::find::existing::with_conversion::Error,
),

Expand All @@ -39,6 +39,14 @@ pub enum GixError {
#[error("gix::reference::head_tree_id::Error error: {0}")]
ReferenceHeadTreeId(#[from] gix::reference::head_tree_id::Error),

///
#[error("gix::reference::iter::Error error: {0}")]
ReferenceIter(#[from] gix::reference::iter::Error),

///
#[error("gix::reference::iter::init::Error error: {0}")]
ReferenceIterInit(#[from] gix::reference::iter::init::Error),

///
#[error("gix::revision::walk error: {0}")]
RevisionWalk(#[from] gix::revision::walk::Error),
Expand Down Expand Up @@ -251,6 +259,18 @@ impl From<gix::reference::head_tree_id::Error> for Error {
}
}

impl From<gix::reference::iter::Error> for Error {
fn from(error: gix::reference::iter::Error) -> Self {
Self::Gix(GixError::from(error))
}
}

impl From<gix::reference::iter::init::Error> for Error {
fn from(error: gix::reference::iter::init::Error) -> Self {
Self::Gix(GixError::from(error))
}
}

impl From<gix::revision::walk::Error> for Error {
fn from(error: gix::revision::walk::Error) -> Self {
Self::Gix(GixError::from(error))
Expand Down
9 changes: 9 additions & 0 deletions asyncgit/src/sync/commits_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,15 @@ impl From<gix::ObjectId> for CommitId {
}
}

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");
Copy link

Choose a reason for hiding this comment

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

(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.


Self::new(oid)
}
}

impl From<CommitId> for gix::ObjectId {
fn from(id: CommitId) -> Self {
Self::from_bytes_or_panic(id.0.as_bytes())
Expand Down
77 changes: 24 additions & 53 deletions asyncgit/src/sync/tags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
use super::{get_commits_info, CommitId, RepoPath};
use crate::{
error::Result,
sync::{repository::repo, utils::bytes2string},
};
use crate::{error::Result, sync::repository::repo};
use scopetime::scope_time;
use std::{
collections::{BTreeMap, HashMap, HashSet},
ops::Not,
};
use std::collections::{BTreeMap, HashMap, HashSet};

///
#[derive(Clone, Hash, PartialEq, Eq, Debug)]
Expand Down Expand Up @@ -64,52 +58,29 @@ pub fn get_tags(repo_path: &RepoPath) -> Result<Tags> {
}
};

let repo = repo(repo_path)?;

repo.tag_foreach(|id, name| {
if let Ok(name) =
// skip the `refs/tags/` part
String::from_utf8(name[10..name.len()].into())
{
//NOTE: find_tag (using underlying git_tag_lookup) only
// works on annotated tags lightweight tags `id` already
// points to the target commit
// see https://github.com/libgit2/libgit2/issues/5586
let commit = repo
.find_tag(id)
.and_then(|tag| tag.target())
.and_then(|target| target.peel_to_commit())
.map_or_else(
|_| {
if repo.find_commit(id).is_ok() {
Some(CommitId::new(id))
} else {
None
}
},
|commit| Some(CommitId::new(commit.id())),
);

let annotation = repo
.find_tag(id)
.ok()
.as_ref()
.and_then(git2::Tag::message_bytes)
.and_then(|msg| {
msg.is_empty()
.not()
.then(|| bytes2string(msg).ok())
.flatten()
});

if let Some(commit) = commit {
adder(commit, Tag { name, annotation });
}

return true;
let gix_repo: gix::Repository =
gix::ThreadSafeRepository::discover_with_environment_overrides(repo_path.gitpath())
Copy link

Choose a reason for hiding this comment

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

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?

.map(Into::into)?;
let platform = gix_repo.references()?;
for mut reference in (platform.tags()?).flatten() {
let commit = reference.peel_to_commit();
let tag = reference.peel_to_tag();
Comment on lines +66 to +67
Copy link

Choose a reason for hiding this comment

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

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!


if let Ok(commit) = commit {
let tag_ref = tag.as_ref().map(gix::Tag::decode);

let name = match tag_ref {
Ok(Ok(tag)) => tag.name.to_string(),
_ => reference.name().shorten().to_string(),
};
let annotation = match tag_ref {
Ok(Ok(tag)) => Some(tag.message.to_string()),
_ => None,
};

adder(commit.into(), Tag { name, annotation });
}
false
})?;
}

Ok(res)
}
Expand Down
Loading