Skip to content

Commit

Permalink
Add patch series support to show & int cmds
Browse files Browse the repository at this point in the history
Add patch series support to the show & integrate commands so that users
would be able to show & integrate an individual patch or a patch series.
As part of this I also added a new hook to integrate that gets when not
using the --force switch. It is called `integrate_verify` the intent
with it is that you could use it to check the status of a PR from a
remote systems like GitHub to prevent you from integrating. This might
be useful to check the CI status of a PR or Approval status of a PR for
example.

Supporting patch series for the show command was pretty straight
forward. However, the integrate command was another thing. The way we
had it implemented previously wasn't very clear what the distinction
between --force and non-force were. Also all of the checks in the
non-force were previosly written explicitly as if there was one patch
and not a series. So, I effectively restructured and rewrote the entire
thing.

A few things that I determined while designing this.

Similiar with all the other commands the user should have to think as
little as possible about the concept of branches and focus primarily on
the concept of patches. In this context this primarily means that the
state of the patches in patch stack is what should be integrated into
the upstream. So, I decided that we always want to create/replace the
request review branch so that it has the most recent state from the
patch stack prior to the actual push. This is different from the
previous implementation where it only did this when the --force switch
was present.

I also decided that --force effectively means to skip the safety checks
that make sure the patches in the patch stack match the patches in the
remote branch prior to create/replace the request review branch. This is
different than the previous implementation as in the previous
implementation the create/replace the request review branch would only
happen if the --force switch was passed.

The following is an outline of how integrate should conceptually be
working now.

- validate patch indexes are within bounds
- add patch ids
- prompt_for_reassurance (based on config)
- git fetch - update the knowledge of the remotes
- if NOT force
    - figure out associated branch
    - verify has associated branch, exit with error
    - check to make sure patches match between stack & remote
    - TODO: in the future: execute hook to verify PR approval & CI status
- create/replace the request review branch
    - in fresh case, it creates the branch, in existing case it updates it to the latest state from ps
- verify isolation (verifies cherry-pick cleanly but also verify isolation hook passes)
- publish the patch(es) from local patch branch up to patch stack upstream
- execute integrate post push hook
- optionally (based on config) delete remote if exists request review branch
- optionally (based on config) delete local request review branch
- optionnaly pull (based on config)

[changelog]
added: patch series support to the show command
added: patch series support to the integrate command
added: integrate_verify hook to the integrate command

<!-- ps-id: 09ca28df-b891-41c0-b36b-aedf30f4b83a -->
  • Loading branch information
drewdeponte committed Oct 19, 2023
1 parent ea3ab90 commit 811dddb
Show file tree
Hide file tree
Showing 10 changed files with 574 additions and 578 deletions.
4 changes: 2 additions & 2 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub struct RequestReviewBranchCmdOpts {

#[derive(Debug, Args)]
pub struct IntegrateCmdOpts {
pub patch_index: usize,
pub patch_index_or_range: String,
/// Use the provided branch name instead of generating one
#[arg(short = 'n')]
pub branch_name: Option<String>,
Expand All @@ -53,7 +53,7 @@ pub struct IntegrateCmdOpts {

#[derive(Debug, Args)]
pub struct ShowCmdOpts {
pub patch_index: usize,
pub patch_index_or_range: String,
}

#[derive(Debug, Args)]
Expand Down
162 changes: 139 additions & 23 deletions src/commands/integrate.rs
Original file line number Diff line number Diff line change
@@ -1,44 +1,160 @@
use super::patch_index_range::PatchIndexRange;
use super::utils::print_err;
use gps as ps;
use std::str::FromStr;

pub fn integrate(
patch_index: usize,
patch_index_or_range: String,
force: bool,
keep_branch: bool,
branch_name: Option<String>,
color: bool,
) {
match ps::integrate::integrate(patch_index, force, keep_branch, branch_name, color) {
Ok(_) => {}
Err(ps::integrate::IntegrateError::PatchIsBehind) => {
print_err(
match PatchIndexRange::from_str(&patch_index_or_range) {
Ok(patch_index_range) => {
match ps::integrate::integrate(
patch_index_range.start_index,
patch_index_range.end_index,
force,
keep_branch,
branch_name,
color,
r#"
The patch you are attempting to integrate is behind.
) {
Ok(_) => {}

This means that some change has been integrated into upstream since you last
sync'd or requested review of this patch.
Err(ps::integrate::IntegrateError::HasNoAssociatedBranch) => {
print_err(
color,
r#"
Looks like the patch(es) haven't had a branch associated with them yet.
To get caught up just sync or request-review the patch again.
"#,
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::IsolationVerificationFailed(
ps::VerifyIsolationError::IsolateFailed(ps::IsolateError::UncommittedChangesExist),
)) => {
print_err(
color,
r#"
You can associate them with gps request-review-branch and gps sync.
Or you can skip saftey checks like this one with --force.
"#,
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::AssociatedBranchAmbiguous) => {
print_err(
color,
r#"
Looks like the patch(es) are associated to more than one branch.
To integrate it needs to know which associated branch you want to work with. You can specify the branch with -n.
Alternatively you can also skip saftey checks like this one with --force.
"#,
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::UpstreamBranchInfoMissing) => {
print_err(
color,
r#"
Looks like the patch(es) have not been pushed yet.
Either push them with gps sync or gps request-review.
Alternatively you can also skip saftey checks like this one with --force.
"#,
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::CommitCountMissmatch(
patch_series_count,
remote_branch_commit_count,
)) => {
print_err(
color,
&format!(
r#"
Looks like the patch series consists of {} patches but the remote has {}.
Investigate this mismatch & resolve it before trying to integrate again.
Or you can skip saftey checks like this one with --force.
"#,
patch_series_count, remote_branch_commit_count
),
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::PatchAndRemotePatchIdMissmatch(patch_index)) => {
print_err(
color,
&format!(
r#"
The patch at index {} has a different patch identifier than the patch on the remote.
It could be that you made a change locally to that patches id or that someone made a change to that patches id remotely.
If you made the change locally, you can update the remote with gps sync or gps request-review.
Alternatively you can also skip saftey checks like this one with --force.
"#,
patch_index
),
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::PatchDiffHashMissmatch(patch_index)) => {
print_err(
color,
&format!(
r#"
The patch at index {} is different from that patch on the remote.
It could be that you made a change locally to that patch or that someone made a change to that patch remotely.
If you made a change locally, you can update the remote with gps sync or gps request-review.
Alternatively you can also skip saftey checks like this one with --force.
"#,
patch_index
),
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::VerifyHookExecutionFailed(e)) => {
print_err(
color,
&format!(
r#"
The integrate_verify hook execution failed with {:?}.
We have aborted the integration. For the integration to continue the integrate_verify hook must exit 0.
Alternatively you can also skip saftey checks like this one with --force.
"#,
e
),
);
std::process::exit(1);
}
Err(ps::integrate::IntegrateError::IsolationVerificationFailed(
ps::VerifyIsolationError::IsolateFailed(
ps::IsolateError::UncommittedChangesExist,
),
)) => {
print_err(
color,
r#"
gps integrate command requires a clean working directory when verifying isolation, but it looks like yours is dirty.
It is recommended that you create a WIP commit. But, you could also use git stash if you prefer.
"#,
);
std::process::exit(1);
);
std::process::exit(1);
}
Err(e) => {
print_err(color, format!("\nError: {:?}\n", e).as_str());
std::process::exit(1);
}
}
}
Err(e) => {
print_err(color, format!("\nError: {:?}\n", e).as_str());
eprintln!("Error: {:?}", e);
std::process::exit(1);
}
}
Expand Down
18 changes: 14 additions & 4 deletions src/commands/show.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,21 @@
use super::patch_index_range::PatchIndexRange;
use gps as ps;
use std::str::FromStr;

pub fn show(patch_index: usize) {
match ps::show(patch_index) {
Ok(_) => (),
pub fn show(patch_index_or_range: String) {
match PatchIndexRange::from_str(&patch_index_or_range) {
Ok(patch_index_range) => {
match ps::show(patch_index_range.start_index, patch_index_range.end_index) {
Ok(_) => (),
Err(e) => {
eprintln!("Error: {:?}", e);
std::process::exit(1);
}
};
}
Err(e) => {
eprintln!("Error: {:?}", e);
std::process::exit(1);
}
};
}
}
4 changes: 2 additions & 2 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn main() {
)
}
cli::Command::Integrate(opts) => commands::integrate::integrate(
opts.patch_index,
opts.patch_index_or_range,
opts.force,
opts.keep_branch,
opts.branch_name,
Expand All @@ -50,7 +50,7 @@ fn main() {
cli::Command::BatchRequestReview(opts) => {
commands::batch_request_review::batch_request_review(opts.patch_index, cli.color)
}
cli::Command::Show(opts) => commands::show::show(opts.patch_index),
cli::Command::Show(opts) => commands::show::show(opts.patch_index_or_range),
cli::Command::Sync(opts) => {
commands::sync::sync(opts.patch_index_or_range, opts.branch_name)
}
Expand Down

0 comments on commit 811dddb

Please sign in to comment.