Skip to content
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.

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

Add gitoxide feature #52

Closed
wants to merge 2 commits into from
Closed

Conversation

SidneyDouw
Copy link

@SidneyDouw SidneyDouw commented Feb 10, 2023

Hi there,

We saw that built depends on the git2 crate to populate some of its GIT_* related variables. Have you considered using gitoxide for that purpose?

In general, gitoxide aims to provide the same functionality as git2 (and much more beyond that). It is built entirely in rust from the ground up, with a high focus on correctness, usability, and performance.

Benefits

  • gitoxide is generally faster than git2 and uses fewer system resources.
  • gitoxide aims to offer more git functionality than git2.
  • gitoxide offers a friendlier API interface and documentation.
  • git-repository with its max-performance-safe feature does not depend on any C build tooling / libraries.
  • Direct participation in upstream improvements or feature requests.
  • Direct support from @Byron and myself until gitoxide's first major release.

Considerations

  • Currently, gitoxide is still missing the capability of doing a diff between worktree and index (aka git status). However, this feature will be available within the first half of 2023.
  • Compile times for built with gitoxide will be higher, as for the moment git2 is still required for git status functionality. This will change and compile times should be about the same as soon as git status is available in gitoxide.

Request for Feedback

I went ahead and added an implementation for gitoxide behind a feature toggle to show what it could look like.
There are a few opportunities to refactor and make this code more maintainable, but I first wanted to showcase the changes in a more simple way and I would be happy to adjust this if this PR moves along.

I would appreciate your feedback if there is any interest in integrating gitoxide into built, and if not I am also happy to close this PR.

Thank you for your time

@lukaslueg
Copy link
Owner

CI seems to be legit?!

---- gitoxide_impls::tests::parse_git_repo stdout ----
thread 'gitoxide_impls::tests::parse_git_repo' panicked at 'range end index 8 out of range for slice of length 4', C:\Users\appveyor\.cargo\registry\src\github.com-1285ae84e5963aae\git-hashtable-0.1.1\src\lib.rs:24:41

@SidneyDouw
Copy link
Author

SidneyDouw commented Feb 12, 2023

You're right, I will see if I can reproduce this error on an i686-windows machine. Will report back here as soon as I find something, sorry about the delay!

The tests running on Ubuntu seem to pass and x86_64-windows on appveyor also seem to run fine though :)

@lukaslueg
Copy link
Owner

Thanks for the initiative.

I can't say I'm overly enthusiastic about the proposal, as the user-facing features derived from git2 are so modest: All we need is a status, a tag, and some other basic info about the repository; we don't really care about merge performance or any of that stuff, while reliability is paramount. I don't want built to crash on people because a dependency's dependency panics on a broken/weird git repo.
For this to be merged, I'd also like gitoxide to completely instead of partially replace the git2 dependency, to keep compile times low.

@Byron
Copy link

Byron commented Feb 13, 2023

You're right, I will see if I can reproduce this error on an i686-windows machine. Will report back here as soon as I find something, sorry about the delay!

The tests running on Ubuntu seem to pass and x86_64-windows on appveyor also seem to run fine though :)

Oh, that's a bummer. gitoxide chose to not support 32 bit systems at all, and even though it might work it definitely would support only smaller repos with up to 2/4GB pack files. In any case, it's not tested.
I am willing to change this stance though thinking that thanks to Rust it should be no problem to at least build correctly and run within the constraints of 32 bit systems.

I don't want built to crash on people because a dependency's dependency panics on a broken/weird git repo.

gitoxide is used in starship which is now able to open any kind of valid git repository which wasn't the case while it was using git2. Thus the opposite will be true.

For this to be merged, I'd also like gitoxide to completely instead of partially replace the git2 dependency, to keep compile times low.

Thanks for that :)! And this is exactly why we integrate into as many and diverse projects as possible. gitoxide should satisfy all their needs and if it doesn't, we will make it happen. Now that there is a motivation to support (and test) 32 bit systems, I think it should happen.

Lastly, please don't take this PR as any kind of pressure - as far as I am concerned I am already happy if gitoxide could fit in here even if ultimately the PR doesn't land for whichever reason. I am thankful for whichever help and guidance you can provide as gitoxide will get closer to where it needs to be thanks to it, and for that I am grateful.

@lukaslueg
Copy link
Owner

@Byron All your comments are fair points and I welcome the initiative. My words of advice would be a) not to overburden the gitoxide project with goals that are not yours (if you do not want to support 32bit platforms, that's fine), and b) completely independently of this PR to make sure that potential users of gitoxide understand at an early stage that the project currently does not support those platforms (if you don't already); having said that, you may want to use std::compile_error as a safeguard on unsupported platforms, so users of gitoxide become aware early on instead of by means of a crash in one of the dependencies.

@Byron
Copy link

Byron commented Feb 13, 2023

All your comments are fair points and I welcome the initiative.
🙏❤️

a) not to overburden the gitoxide project with goals that are not yours (if you do not want to support 32bit platforms, that's fine) [..]

Thank you, I will be sure to choose wisely. From my experience it's all about priorities in the end as a lot of features are needed and there is no way around them, and wanting to integrate with another project is usually great to change priorities to make it work.

[..] you may want to use std::compile_error as a safeguard on unsupported platforms, so users of gitoxide become aware early on instead of by means of a crash in one of the dependencies.

gitoxide has to be able to replace git2 for the majority of use-cases, if not all of them, and I tend to take shortcuts to focus on what seems to matter more in the moment. As time changes, priorities shift and some shortcuts turn out to be tech-debt after all. Just today I learned that cargo also needs to be built on 32 bit platforms (even though they don't seem to be testing it on CI), so there is a lot speaking for building and working on those platforms. git can handle arbitrarily sized pack files there and gitoxide can't, and that is nothing I would want to change - everything else should work and what we see here is an anomaly that definitely deserves fixing.

In any case, if the timeline of getting git status-like functionality until the middle of the year is OK with you, we could be back when the PR can be completed to wrap it up.

@SidneyDouw
Copy link
Author

SidneyDouw commented Apr 18, 2023

A quick update: gitoxide should now have support for 32-bit systems.
I updated the crate version and the CI checks over on my fork all seem to pass

The checks here fail because cargo cannot download crates due to an SSL connect error.
It looks like this same error occurred during checks of other pushes / PRs as well. I am looking into to it to see if I can find out any more information.

@Byron
Copy link

Byron commented Apr 18, 2023

I think during this time GitHub was broken and I wouldn't be surprised if it would just get through if the job is being restarted.

@lukaslueg
Copy link
Owner

The appveyor failure is due to it still being on an VS2015-image, which I now switched over.

Congratulations on making gitoxide work on 32-bit platforms. 👍

Is there a prospect of replacing git2 in its entirety with gitoxide? That is, having git2 and gitoxide being mutually exclusive features, where either one or the other implementation is used.

@Byron
Copy link

Byron commented Apr 29, 2023

Congratulations on making gitoxide work on 32-bit platforms. 👍

Thank you! It wasn't even that hard. It should be noted that on 32 bit platforms, some limitations apply as packs can't exceed a certain size then.

That is, having git2 and gitoxide being mutually exclusive features, where either one or the other implementation is used.

It's fair to wait until this is possible - gix status is in the works as it is also vital for helix, and once available this PR could make use of it.

Tests would still use git2 though, unless you'd like to switch to using gix-testtools which allows write fixture scripts using git directly which should yield faster builds.

@lukaslueg
Copy link
Owner

Tests would still use git2 though, unless you'd like to switch to using gix-testtools which allows write fixture scripts using git directly which should yield faster builds.

That would entail a de-facto dependency on a git cli?!

@Byron
Copy link

Byron commented Apr 29, 2023

Yes, it leverages the presence of sh and git. The machines that usually run these tests thus far had them pre-installed so it wasn't a problem for gitoxide. built might be different though. The reason I am mentioning it at all is that I personally prefer writing shell scripts, they feel a lot leaner than doing the same thing in Rust with git2.

@lukaslueg
Copy link
Owner

I guess its fair enough to assume that a git cli is present for any built environment in which it is tested

@Byron Byron mentioned this pull request Aug 6, 2023
8 tasks
@Byron
Copy link

Byron commented Aug 6, 2023

Hi @lukaslueg, I have started a new integration PR which will supersede this one when it's posted here later this year. Thus I recommend closing this PR in the meantime. Thank you, and sorry for the delays.

@lukaslueg lukaslueg closed this Aug 6, 2023
@AlJohri
Copy link

AlJohri commented Jan 28, 2024

Looking forward to the gitoxide integration @Byron! I just ran into some compile errors when attempting to use built with the git2 feature flag due to the libgit2-sys dependency and found myself searching for this functionality.

@lukaslueg
Copy link
Owner

@AlJohri Can built do something about that as is?

@Byron
Copy link

Byron commented Jan 29, 2024

@AlJohri I am finally working on git-status-like functionality which will allow the built integration to be finished. This is the PR: GitoxideLabs/gitoxide#1252 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants