fix(git_push): replace force with force_with_lease - #5
Conversation
Codecov ReportAttention: Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
b1ffbc3 to
c230570ComparerootCircle
commented
Jun 10, 2025
Related #4 |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the git push functionality to replace the basic force push flag with a safer force‐with‐lease approach, reducing the risk of accidentally overwriting remote changes. Key changes include:
- Removing the old force push mechanism and introducing the force_with_lease flag and an optional force_with_lease_ref field.
- Adding new validation logic and refspec construction for handling force‐with‐lease pushes.
- Updating error messages to reflect the revised push strategy.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/workflows/default/action/ta09_pull_push.rs | Updated push configuration to use force_with_lease instead of force. |
| src/events/git_push.rs | Replaced the force flag with force_with_lease and added new validation/refspec functions for safe push behavior. |
Comments suppressed due to low confidence (1)
src/events/git_push.rs:271
- Consider adding a comment clarifying the purpose and format of the refspec string that uses curly braces to denote the expected OID for force‐with‐lease. This will help future maintainers understand the string construction and its relation to git push semantics.
Ok(format("+{}^{{{}}}", base_refspec, expected_ref))
Uh oh!
There was an error while loading. Please reload this page.
| pub fn set_force_with_lease(&mut self, force_with_lease: bool) -> &mut Self { | ||
| self.force_with_lease = force_with_lease; | ||
| self | ||
| } | ||
| #[allow(dead_code)] | ||
| pub fn set_force_with_lease_ref(&mut self, ref_name: Option<String>) -> &mut Self { | ||
| self.force_with_lease_ref = ref_name; | ||
| self | ||
| } |
There was a problem hiding this comment.
nitpicky thing, but whenever with a method, you pass the ref and return the value itself (not the ref), it generally comes under builder pattern. (different from getter/setter)
for that method prefix their names with with_ instead of set_
| pub pre_check_rules: Vec<Box<dyn Rule + Send + Sync>>, | ||
| pub force: bool, | ||
| pub force_with_lease: bool, | ||
| pub force_with_lease_ref: Option<String>, |
There was a problem hiding this comment.
Why do we actually need this from user POV? Trying to think about the usecase here for force_with_lease_ref
There was a problem hiding this comment.
It just came up when I was searching for force_with_lease. Thought it might be useful in some scenario.
So i added in the PR to be reviewed 😅
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
c991af3 to
1223ff8Compare
Replaced the basic
forceflag withforce-with-leasefunctionality to prevent accidental overwrites of remote changes.Added
force_with_lease: booland optionalforce_with_lease_ref: Option<String>fieldforce_with_lease_ref: allows specifying exact commit hash the remote should be at