Check remote tracking branches when generating canned names #12121
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
mtsgrd
force-pushed
the
remote-aware-canned-branch
branch
from
b2059d0 to
c7a6f9b
Compare
Ensure generated canned branch names avoid conflicts with remote tracking refs as well as local branches. Add new functions in but-core to find unique refnames with an explicit remote check and a canned-ref wrapper that uses it. Update but-api to consult the project's default push remote and use the remote-aware generation when available. Include tests covering remote-aware behavior to ensure names that exist on remotes are skipped.
mtsgrd
force-pushed
the
remote-aware-canned-branch
branch
from
c7a6f9b to
1f1e612
Compare
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
It turns out I have a lot of comments, but also learned a lot on what still needs to be done to have an "AI-safe" codebase. VirtualBranchesHandle, I am looking at you :P.
A couple of things I think could address before I give it the finishing touches.
PS: Something I don't understand it where conflicts are coming from. The unique names are unique, so refs/heads/feat-1 will not conflict. And if the problem is that there may be refs/remotes/origin/feat-1 which then seems to be the remote tracking branch of refs/heads/feat-1, which will then be inferred by but-graph logic as local/remote tracking branch pair… then I think at least at some point in the future we should consider to remove that but-graph logic. After all, it was just added to deal with GitButler not setting up tracking previously, but when the new apply is used, that will now happen as well. So I think this context I'd also add to the docs of the function to mark it clearly as "shoudln't be needed once but-graph changed its ways.
| pub fn find_unique_refname_with_remote_check( | ||
| repo: &gix::Repository, | ||
| template: &gix::refs::FullNameRef, | ||
| remote_name: &str, |
There was a problem hiding this comment.
I think there is a type for that, probably gix::remote::Name.
| if repo.try_find_reference(&local_ref)?.is_some() { | ||
| return Ok(true); | ||
| } | ||
| let remote_ref = format!("refs/remotes/{remote_name}/{}", name.to_str_lossy()); |
There was a problem hiding this comment.
Can't use to_str_lossy() because we can't loose information here.
| .category() | ||
| .with_context(|| format!("Input branch {template} could not be categorized"))?; | ||
|
|
||
| let ref_exists = |name: &bstr::BStr| -> anyhow::Result<bool> { |
There was a problem hiding this comment.
I think it will help the code to take a FullNameRef here.
| return Ok(template.to_owned()); | ||
| } | ||
|
|
||
| // Extract base name and number if it already has a numerical suffix |
There was a problem hiding this comment.
This looks like a duplicate, let's let there be only one way of generating these names.
| pub fn canned_branch_name(ctx: &Context) -> Result<String> { | ||
| let rn = but_core::branch::unique_canned_refname(&*ctx.repo.get()?)?; | ||
| let repo = ctx.repo.get()?; | ||
| let state = VirtualBranchesHandle::new(ctx.project_data_dir()); |
There was a problem hiding this comment.
Seeing this showing up just tells me I have to work on getting rid of these handles so it won't be picked up anymore.
Technically it's legacy, but that doesn't mean we can keep writing insta-legacy code. Instead it means it should become modern.
Here one would use let (_guard, repo, ws, _) = ctx.workspace_and_db()? and ask ws for the target information.
|
|
||
| #[test] | ||
| fn avoids_remote_tracking_branches() -> anyhow::Result<()> { | ||
| let (repo, _tmp) = writable_scenario("unborn-empty"); |
There was a problem hiding this comment.
It's my own fault to see this repeated, but in the final review I'd definitely change the pattern of using writable fixtures for read-only functions, and instead create a fixture just for it which is presented before beforehand via but-testsupport.
| // Try to get the push remote name to avoid conflicts with remote branches | ||
| let state = VirtualBranchesHandle::new(project_data_dir); |
There was a problem hiding this comment.
We must use ws to check the target information.
| but-workspace = { workspace = true, features = ["legacy"] } | ||
| but-hunk-dependency.workspace = true | ||
| but-ctx.workspace = true | ||
| gitbutler-stack.workspace = true |
There was a problem hiding this comment.
Just yesterday I removed this 😭🤣.
|
Thanks @Byron, I'll address these comments and mark it ready for review when I get there! |
2 participants
Ensure generated canned branch names avoid conflicts with remote
tracking refs as well as local branches. Add new functions in but-core
to find unique refnames with an explicit remote check and a canned-ref
wrapper that uses it. Update but-api to consult the project's default
push remote and use the remote-aware generation when available. Include
tests covering remote-aware behavior to ensure names that exist on
remotes are skipped.