gitoxide update #705
gitoxide update #705
Conversation
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 Report
@@ 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
Continue to review full report at Codecov.
|
|
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 |
There was a problem hiding this comment.
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?
| self.repo | ||
| .committer() | ||
| .map(|c| c.name.to_string()) | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -224,53 +218,39 @@ impl Repo { | |||
| } | |||
|
|
|||
| pub fn get_name_and_url(&self) -> Result<(String, String)> { | |||
There was a problem hiding this comment.
Should the return Ok(Default::default()) be changed to return Errs?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Got it, I'm OK with the existing code, then 😃 👍
Looking forward to it!
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 |
|
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.
|
Yes, that's right :D. It's also the second attempt at naming the project, which was called |
Tasks
repo.config()instead ofgit2config in all places