gitoxide update #705

Byron · 2022-07-22T14:39:32Z

Tasks

update
use repo.config() instead of git2 config in all places

It also makes use of the `GIT_COMMITTER|USER_*` environment variables, leading to it being more true to the intent while finding the name that git would use, too.

This is a more literal translation and some more cleanup steps have to follow.

`git2` is now used only to find worktree changes, an area where great speedups could be achieved if it was implemented by gitoxide.

codecov-commenter · 2022-07-23T01:09:18Z

Codecov Report

Merging #705 (f965965) into main (debd453) will increase coverage by 0.17%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #705      +/-   ##
==========================================
+ Coverage   13.36%   13.53%   +0.17%     
==========================================
  Files          19       19              
  Lines        1160     1145      -15     
==========================================
  Hits          155      155              
+ Misses       1005      990      -15

Impacted Files	Coverage Δ
src/info/mod.rs	`0.00% <0.00%> (ø)`
src/info/repo.rs	`16.66% <0.00%> (+1.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update debd453...f965965. Read the comment docs.

Byron · 2022-07-23T01:10:34Z

I think it's ready for review. The changes were tested on MacOS and Windows 11 just to be sure the repo name can indeed be figured out with the refactored code.

Now the last bastion of gitoxide to conquer is worktree changes, and I am looking forward to that. As mentioned before, it might not happen this year, but it's not for a lack of motivation. Let's see :).

spenserblack

This is awesome work, and a huge step towards making onefetch more oxidized (I finally realized that the name is a reference to a "Rusty" version of git, right?) 🦀 🎉

I don't want this to block merging, but this PR might be a good opportunity to start adding some tests. What do you think the best testing strategy would be? Generating temporary repos? Mocking gitoxide functions/methods? Or something else?

spenserblack · 2022-07-23T02:23:25Z

src/info/repo.rs

+        self.repo
+            .committer()
+            .map(|c| c.name.to_string())
+            .unwrap_or_default()


What do you think about defaulting to something like "[UNKNOWN]" if we cannot unwrap the committer? Technically a user.name could be [UNKNOWN], but I think it's unlikely enough 😆

Or if we stick with the default, why not use committer_or_default?

A great catch, for this round I left it functionally similar, so if nothing is configured there are empty strings. This behaviour is also present in repo_name_and_url() even though null would probably bode better in with json output, I don't know if the UI has any special handling for these empty strings though. Generally I'd certainly prefer options.

This also answers why committer_or_default() wasn't chosen - you actually get an entirely made up user name "gitoxide <gitoxide@localhost>" which probably is misleading - it's akin to what git does if nothing is configured, using the current user name along with some way of figuring out a local email address.

To sum it up, I'd prefer returning Option instead of empty strings and have the UI decide how to display these.

for this round I left it functionally similar

Got it, that's probably for the best 👍
Sorry, I should have checked the original behavior more closely

spenserblack · 2022-07-23T02:31:22Z

src/info/repo.rs

@@ -224,53 +218,39 @@ impl Repo {
    }

    pub fn get_name_and_url(&self) -> Result<(String, String)> {


Should the return Ok(Default::default()) be changed to return Errs?

I think this would break onefetch as it aborts the information retrieval process for repository information entirely. If there was an improvement to make, I'd certainly go for Result<Option<(…)>> here.

Got it, I'm OK with the existing code, then 😃 👍

o2sh · 2022-07-24T08:09:16Z

Now the last bastion of gitoxide to conquer is worktree changes, and I am looking forward to that. As mentioned before, it might not happen this year, but it's not for a lack of motivation. Let's see :).

Looking forward to it!

I don't want this to block merging, but this PR might be a good opportunity to start adding some tests. What do you think the best testing strategy would be? Generating temporary repos? Mocking gitoxide functions/methods? Or something else?

I'll let you address this part of @spenserblack's comment. I don't mind merging the PR as is but this is typically the kind of work that needs non regression tests. You can just give us some pointers and we will work on it as part of #700

Byron · 2022-07-25T02:23:32Z

Oh, sorry, I completely and accidentally skipped over @spenserblack 's suggestion about adding some tests. Also I am happy to pick up on @o2sh to make suggestions instead of adding tests.

gitoxide mostly uses real git repositories for testing, and has amassed a set of utilities that are available via git-testtools. The most important one here I imagine would be scripted_fixture_repo_read_only(…) which executes some script while keeping the result locally available and dealing with re-executing it if its contents changes. That way, one could write shell scripts that clone repositories or set them up which can then be fed into Repo::new(fixture) to assert on the result accordingly. The system also comes with an archive feature that in conjunction with git-lfs helps to speed up CI as it won't have to clone repositories all the time anymore (instead, it downloads from git-lfs). That however, is optional and it's possible to disable archive creation entirely using .gitignore files. Now that I write it here I notice that the archival functionality is definitely still under-documented. I will correct that shortly (maybe by the time you read it it's already done).

Byron · 2022-07-26T01:13:47Z

This is awesome work, and a huge step towards making onefetch more oxidized (I finally realized that the name is a reference to a "Rusty" version of git, right?) 🦀 🎉

Yes, that's right :D. It's also the second attempt at naming the project, which was called grit before, it's git, but with some rust inserted. The motivation to change the name was the programs name, grit, which took too long to type and gix is a little better even though I think most won't ever use it. Ideally, one day people prefer ein over git for typical workflows, but I digress :).

Byron added 5 commits July 22, 2022 22:34

update to git-repository 0.20 (and the latest git-config)

58da2df

obtain user name with gitoxide

38fbfec

It also makes use of the `GIT_COMMITTER|USER_*` environment variables, leading to it being more true to the intent while finding the name that git would use, too.

obtain remote url and repo name using gitoxide

b1c80e8

This is a more literal translation and some more cleanup steps have to follow.

remote git2 repository from repo module.

881f514

`git2` is now used only to find worktree changes, an area where great speedups could be achieved if it was implemented by gitoxide.

more robust parsing of repo name

f965965

Byron marked this pull request as ready for review July 23, 2022 01:10

spenserblack approved these changes Jul 23, 2022

View reviewed changes

o2sh mentioned this pull request Jul 25, 2022

Improve test coverage of onefetch #700

Open

32 tasks

o2sh merged commit 2bacf5a into o2sh:main Jul 25, 2022

Byron deleted the gitoxide-update branch July 26, 2022 00:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gitoxide update #705

gitoxide update #705

Byron commented Jul 22, 2022 •

edited

Loading

codecov-commenter commented Jul 23, 2022

Byron commented Jul 23, 2022

spenserblack left a comment

spenserblack Jul 23, 2022

Byron Jul 23, 2022

spenserblack Jul 23, 2022

spenserblack Jul 23, 2022

Byron Jul 23, 2022

spenserblack Jul 23, 2022

o2sh commented Jul 24, 2022

Byron commented Jul 25, 2022

Byron commented Jul 26, 2022

		@@ -224,53 +218,39 @@ impl Repo {
		}

		pub fn get_name_and_url(&self) -> Result<(String, String)> {

gitoxide update #705

gitoxide update #705

Conversation

Byron commented Jul 22, 2022 • edited Loading

Tasks

codecov-commenter commented Jul 23, 2022

Codecov Report

Byron commented Jul 23, 2022

spenserblack left a comment

Choose a reason for hiding this comment

spenserblack Jul 23, 2022

Choose a reason for hiding this comment

Byron Jul 23, 2022

Choose a reason for hiding this comment

spenserblack Jul 23, 2022

Choose a reason for hiding this comment

spenserblack Jul 23, 2022

Choose a reason for hiding this comment

Byron Jul 23, 2022

Choose a reason for hiding this comment

spenserblack Jul 23, 2022

Choose a reason for hiding this comment

o2sh commented Jul 24, 2022

Byron commented Jul 25, 2022

Byron commented Jul 26, 2022

Byron commented Jul 22, 2022 •

edited

Loading