Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
cococonscious
left a comment
cococonscious
left a comment
There was a problem hiding this comment.
Please address the open comments, they're mostly the same as in #162 .
Also, please update/rebase your branch.
| if !scopes.contains(&scope) { | ||
| scopes.push(scope); | ||
| } | ||
| // Iterate through commits |
There was a problem hiding this comment.
The comments in this file are a bit redundant.
| temp_dir: &std::path::Path, | ||
| message: &'static str, | ||
| ) -> Result<(), Box<dyn Error>> { | ||
| Command::new("git") |
There was a problem hiding this comment.
Could you try to do this programatically (like before with git2) instead of running a command?
Cargo.toml
Outdated
|
|
||
| [features] | ||
| vendored-openssl = ["git2/vendored-openssl"] | ||
| vendored-openssl = [] |
There was a problem hiding this comment.
I think you can remove this feature now since there's no dependencies in it anymore. Does this change mean that openssl isn't a requirement for koji anymore?
There was a problem hiding this comment.
Also, could you please not include the default features for gitoxide and only use those that are actually used in koji? See https://docs.rs/gix/latest/gix/#library-developers
There was a problem hiding this comment.
I removed the dependency, but respectfully dead code elimination handles most of the incurred bloat — I'm not particularly interested in the complexities of gix's flags; and don't believe this is blocking.
There was a problem hiding this comment.
I assume that explicitly defining the required features, instead of relying on DCE, would improve build times, is this assumption not correct?
There was a problem hiding this comment.
This assumption is correct; just again, I believe the priority should be merging this long-standing issue; my expertise isn't in the particularities of Gix flags.
|
and unfortunately gix is too low level for some of those functions to not shell out to git; probably in a year or so the gix team will have implemented their high-level wrappers but otherwise we'd be hosting a huge amount of code to just avoid running something that is 1:1 with the use-case (koji is for git commits) |
I honestly fail to see the advantage of moving to gix if we're not implementing the commits ourselves, which would also allow us to move away from cocogitto as well. Moving from a library implementation to wrapping commands feels like a step backwards, e.g. cocogitto is also 1:1 with the usecase of creating commits and they also implement commits programmatically: https://github.com/cocogitto/cocogitto/blob/main/src/git/commit.rs This is a blocking matter for me. |
|
Then I'm misunderstanding -- I thought those git functions were isolated to the tests/integration; Certainly worthwhile in core. Let me dig back in. |
|
Yeah, on second thought if we need add/commit, this just will not work. Sorry for the confusion.
▰▰▰▰▰
Miles Wirht 🙃
… From: Finley Thomalla ***@***.***>
Sent: 05 February 2026 00:28
To: cococonscious/koji ***@***.***>
Cc: Miles Wirht ***@***.***>, Author ***@***.***>
Subject: Re: [cococonscious/koji] Gitoxide integration (PR #174)
cococonscious left a comment (cococonscious/koji#174)
> and unfortunately gix is too low level for some of those functions to not shell out to git; probably in a year or so the gix team will have implemented their high-level wrappers but otherwise we'd be hosting a huge amount of code to just avoid running something that is 1:1 with the use-case (koji is for git commits)
I honestly fail to see the advantage of moving to gix if we're not implementing the commits ourselves, which would also allow us to move away from cocogitto as well. Moving from a library implementation to wrapping commands feels like a step backwards, e.g. cocogitto is also 1:1 with the usecase of creating commits and they also implement commits programmatically: https://github.com/cocogitto/cocogitto/blob/main/src/git/commit.rs
This is a blocking matter for me.
--
Reply to this email directly or view it on GitHub:
#174 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
You're not necessarily misunderstanding, it's just that I want to replace cocogitto, which koji is using to create commits at the moment, with our own implementation at some point, which is why it'd be great to have the basis for it it here already. |
|
I get that 100% — the biggest issue is the lack of functionality for adding to the index.
See the tracking issue for gitui: GitoxideLabs/gitoxide#1078.
If you're willing to keep cocogitto in the short term, and keep Gix for the internal traversal, git (or I can move back to git2) for testing. I can try and speed up the proccess for that functionality (think I could PR if necessary). So we can move to Gix in the medium to long term.
Would that be good?
▰▰▰▰▰
Miles Wirht 🙃
… From: Finley Thomalla ***@***.***>
Sent: 06 February 2026 04:47
To: cococonscious/koji ***@***.***>
Cc: Miles Wirht ***@***.***>, Author ***@***.***>
Subject: Re: [cococonscious/koji] Gitoxide integration (PR #174)
cococonscious left a comment (cococonscious/koji#174)
> Then I'm misunderstanding -- I thought those git functions were isolated to the tests/integration; Certainly worthwhile in core. Let me dig back in.
You're not necessarily misunderstanding, it's just that I want to replace cocogitto, which koji is using to create commits at the moment, with our own implementation at some point, which is why it'd be great to have the basis for it it here already.
--
Reply to this email directly or view it on GitHub:
#174 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
2 participants
Had to scrap due to poor git usage