-
Notifications
You must be signed in to change notification settings - Fork 882
feat(api): add optional order parameter to branch creation #12693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -473,6 +473,7 @@ pub fn split_branch( | |
| &refname, | ||
| None, | ||
| None, | ||
| None, | ||
| guard.write_permission(), | ||
| )?; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,6 +189,7 @@ pub mod stacks { | |
| &ref_name, | ||
| Some(remote_ref_name), | ||
| None, | ||
| None, | ||
| )?; | ||
|
|
||
| let repo = ctx.repo.get()?; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1155,6 +1155,7 @@ pub fn split_branch( | |
| &refname, | ||
| None, | ||
| None, | ||
| None, | ||
| guard.write_permission(), | ||
| )?; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,7 @@ impl BranchManager<'_> { | |
| target: &Refname, | ||
| upstream_branch: Option<RemoteRefname>, | ||
| pr_number: Option<usize>, | ||
| order: Option<usize>, | ||
| perm: &mut RepoExclusive, | ||
| ) -> Result<(StackId, Vec<StackId>, Vec<String>)> { | ||
| let branch_name = target | ||
|
|
@@ -129,7 +130,7 @@ impl BranchManager<'_> { | |
| OnWorkspaceMergeConflict::MaterializeAndReportConflictingStacks, | ||
| workspace_reference_naming: WorkspaceReferenceNaming::Default, | ||
| uncommitted_changes: UncommitedWorktreeChanges::KeepAndAbortOnConflict, | ||
| order: None, | ||
| order, | ||
| new_stack_id: None, | ||
| }, | ||
| )?; | ||
|
|
@@ -211,7 +212,7 @@ impl BranchManager<'_> { | |
| .peel_to_commit() | ||
| .context("failed to peel to commit")?; | ||
|
|
||
| let order = vb_state.next_order_index()?; | ||
| let order = order.unwrap_or(vb_state.next_order_index()?); | ||
|
|
||
|
Comment on lines
213
to
216
|
||
| let mut branch = if let Some(mut branch) = vb_state | ||
| .find_by_top_reference_name_where_not_in_workspace(&target.to_string())? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,7 @@ fn rebase_commit() { | |
| &unapplied_branch, | ||
| None, | ||
| None, | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
orderinput isn’t covered by tests yet. It would be good to add acreate_virtual_branch_from_branch(..., order: Some(0))test (and possibly another non-zero case) asserting that the created stack ends up at the requested position and that existing stacks are shifted/renumbered appropriately.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shadowing is real. However, this is idiomatic Rust — shadowing an
Optionwith its unwrapped value is a common and accepted pattern. It's not a bug or a readability problem in Rust conventions, unlike in other languages. The suggestion is a matter of style preference, not a correctness issue.