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

Removing dependency to git2 #220

Merged
merged 7 commits into from
Feb 15, 2024
Merged

Removing dependency to git2 #220

merged 7 commits into from
Feb 15, 2024

Conversation

vsiles
Copy link
Contributor

@vsiles vsiles commented Feb 8, 2024

This is related to #217

git2 is replaced by:

  • in buffrs itself, we now call git status --porcelain directly, since gix is not able to give us information about untracked files.
  • in the test-suite, we use gix instead of git2

The test suite has been updated to deal with some minor behaviour.

I run time cargo build before/after to compare since build time was an issue, here are the results on my laptop (M3):

  • before:
    Finished dev [unoptimized + debuginfo] target(s) in 57.88s

________________________________________________________
Executed in   57.95 secs    fish           external
   usr time  185.65 secs    0.16 millis  185.65 secs
   sys time   55.63 secs    1.15 millis   55.63 secs
  • after:

   Finished dev [unoptimized + debuginfo] target(s) in 15.59s

________________________________________________________
Executed in   15.66 secs    fish           external
  usr time  102.08 secs    0.18 millis  102.08 secs
  sys time   12.89 secs    1.19 millis   12.89 secs

git2 is replaced by:
- in buffrs itself, we now call `git status --porcelain` directly,
  since `gix` is not able to give us information about untracked files.
- in the test-suite, we use `gix` instead of `git2`
Cargo.toml Outdated Show resolved Hide resolved
src/command.rs Show resolved Hide resolved
@vsiles vsiles marked this pull request as ready for review February 8, 2024 16:47
@mara-schulke
Copy link
Contributor

Hi, is it possible to use gix for this? If not we can remove the feature altogether. Parsing cli outputs is super error prone and I would like to avoid this.

@vsiles
Copy link
Contributor Author

vsiles commented Feb 9, 2024

As I mention in the issue discussion, gix does not yet allow us to do what git2 was doing: gix does not currently have information about untracked files, so we could only complain if some tracked files have changes.
That being said, I took care of calling git status --porcelain because it is made exactly for that:

--porcelain[=]
Give the output in an easy-to-parse format for scripts. This is similar to the short output, but will remain stable across Git > versions and regardless of user configuration. See below for details.

That leaves as multiple options:

  1. we remove this check altogether
  2. we use gix and accept to ignore untracked file (for now at least)
  3. we use git but don't parse the output. If the output is empty, the repo is clean, otherwise it is not and we print the output as is
  4. we trust the --porcelain option and parse the output

I don't have a strong opinion here, so I'll implement what you think is best

src/command.rs Outdated Show resolved Hide resolved
@tomkarw
Copy link
Contributor

tomkarw commented Feb 9, 2024

I agree that using git status --porcelain will be robust enough, so I vote for option 4.

That said, I'm fuzzy on why we care about uncommitted changes in the first place. Maybe it's a chance to skim some overreaching features fat.

@vsiles
Copy link
Contributor Author

vsiles commented Feb 9, 2024

Yes, I think I'm too new to have enough context here ;) I'm leaving that decision to @mara-schulke and you

@tomkarw tomkarw self-requested a review February 9, 2024 16:49
@mara-schulke
Copy link
Contributor

Okay, so im okay with option 4 but: I would not error when the parsing fails. So I would at most emit a warning here that git could not be found / the output is malformed. Currently your implementation raises errors which would potentially abort the call.

If you make it best-effort (say: skip this and emit a warning line to the user) im okay with using --porcelain! 😊

@mara-schulke mara-schulke added complexity::low Issues or ideas with a low implementation cost component::cli Everything related to the buffrs cli integration Changes and ideas related to integrating buffrs with 3rd-party software or tools priority::low Please dont work on this if you can contribute something with a higher priority type::fix labels Feb 12, 2024
@vsiles
Copy link
Contributor Author

vsiles commented Feb 14, 2024

Should be good for review now. Thanks for all the input.

Copy link
Contributor

@tomkarw tomkarw left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge

@vsiles vsiles merged commit 78740b1 into main Feb 15, 2024
7 checks passed
@vsiles vsiles deleted the vsiles/removing-git2-dep branch February 15, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity::low Issues or ideas with a low implementation cost component::cli Everything related to the buffrs cli integration Changes and ideas related to integrating buffrs with 3rd-party software or tools priority::low Please dont work on this if you can contribute something with a higher priority type::fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants