Prevent bad onboarding scenario

Author: Caleb-T-Owens

crates/gitbutler-branch-actions/src/base.rs

@@ -19,7 +19,9 @@ use gitbutler_repo::{
     RepositoryExt,
 };
 use gitbutler_repo_actions::RepoActionsExt;
-use gitbutler_stack::{BranchOwnershipClaims, Stack, Target, VirtualBranchesHandle};
+use gitbutler_stack::{
+    canned_branch_name, BranchOwnershipClaims, Stack, Target, VirtualBranchesHandle,
+};
 use serde::Serialize;
 use tracing::instrument;
 
@@ -196,47 +198,52 @@ pub(crate) fn set_base_branch(
                 },
             );
 
-            let (upstream, upstream_head) = if let Refname::Local(head_name) = &head_name {
-                let upstream_name = target_branch_ref.with_branch(head_name.branch());
-                if upstream_name.eq(target_branch_ref) {
-                    (None, None)
-                } else {
-                    match repo.find_reference(&Refname::from(&upstream_name).to_string()) {
-                        Ok(upstream) => {
-                            let head = upstream
-                                .peel_to_commit()
-                                .map(|commit| commit.id())
-                                .context(format!(
-                                    "failed to peel upstream {} to commit",
-                                    upstream.name().unwrap()
-                                ))?;
-                            Ok((Some(upstream_name), Some(head)))
+            let (upstream, upstream_head, branch_matches_target) =
+                if let Refname::Local(head_name) = &head_name {
+                    let upstream_name = target_branch_ref.with_branch(head_name.branch());
+                    if upstream_name.eq(target_branch_ref) {
+                        (None, None, true)
+                    } else {
+                        match repo.find_reference(&Refname::from(&upstream_name).to_string()) {
+                            Ok(upstream) => {
+                                let head = upstream
+                                    .peel_to_commit()
+                                    .map(|commit| commit.id())
+                                    .context(format!(
+                                        "failed to peel upstream {} to commit",
+                                        upstream.name().unwrap()
+                                    ))?;
+                                Ok((Some(upstream_name), Some(head), false))
+                            }
+                            Err(err) if err.code() == git2::ErrorCode::NotFound => {
+                                Ok((None, None, false))
+                            }
+                            Err(error) => Err(error),
                         }
-                        Err(err) if err.code() == git2::ErrorCode::NotFound => Ok((None, None)),
-                        Err(error) => Err(error),
+                        .context(format!("failed to find upstream for {head_name}"))?
                     }
-                    .context(format!("failed to find upstream for {head_name}"))?
-                }
+                } else {
+                    (None, None, false)
+                };
+
+            let branch_name = if branch_matches_target {
+                canned_branch_name(repo)?
             } else {
-                (None, None)
+                head_name.to_string().replace("refs/heads/", "")
             };
 
             let mut branch = Stack::create(
                 ctx,
-                head_name.to_string().replace("refs/heads/", ""),
+                branch_name,
                 Some(head_name),
                 upstream,
                 upstream_head,
-                gitbutler_diff::write::hunks_onto_commit(
-                    ctx,
-                    current_head_commit.id(),
-                    gitbutler_diff::diff_files_into_hunks(&wd_diff),
-                )?,
+                current_head_commit.tree_id(),
                 current_head_commit.id(),
                 0,
                 None,
                 ctx.project().ok_with_force_push.into(),
-                true, // allow duplicate name since here we are creating a lane from an existing branch
+                !branch_matches_target, // allow duplicate name since here we are creating a lane from an existing branch
             )?;
             branch.ownership = ownership;
 

crates/gitbutler-branch-actions/tests/fixtures/for-details.sh

@@ -41,7 +41,8 @@ git clone remote complex-repo
   $CLI project add --switch-to-workspace "$local_tracking_ref"
   for round in $(seq 10); do
     echo virtual-main >> file
-    $CLI branch commit --message "virt-$round" main
+    # We will now use a canned branch name here
+    $CLI branch commit --message "virt-$round" a-branch-1
   done
 
   git checkout -b non-virtual-feature main
@@ -51,5 +52,8 @@ git clone remote complex-repo
   done
 
   # pretend the remote is at the same state as our local `main`
-  git update-ref refs/remotes/origin/main main
+  # This previously wanted to update the remote to match the local main, we no
+  # longer have a "main" virutal branch, so this has been changed to the canned
+  # branch.
+  git update-ref refs/remotes/origin/main a-branch-1
 )

crates/gitbutler-branch-actions/tests/virtual_branches/init.rs

@@ -76,6 +76,8 @@ fn dirty_target() {
 
     fs::write(repo.path().join("file.txt"), "content").unwrap();
 
+    let old = std::env::var("GIT_AUTHOR_NAME").ok();
+    std::env::set_var("GIT_AUTHOR_NAME", "GitButler");
     gitbutler_branch_actions::set_base_branch(
         ctx,
         &"refs/remotes/origin/master".parse().unwrap(),
@@ -86,7 +88,10 @@ fn dirty_target() {
 
     let stacks = stack_details(ctx);
     assert_eq!(stacks.len(), 1);
-    assert_eq!(stacks[0].1.derived_name, "master");
+    assert_eq!(stacks[0].1.derived_name, "g-branch-1");
+    if let Some(old) = old {
+        std::env::set_var("GIT_AUTHOR_NAME", old);
+    }
 }
 
 #[test]
@@ -150,7 +155,7 @@ fn commit_on_target() {
 
     let stacks = stack_details(ctx);
     assert_eq!(stacks.len(), 1);
-    assert_eq!(stacks[0].1.derived_name, "master");
+    assert_eq!(stacks[0].1.derived_name, "g-branch-1");
     assert_eq!(stacks[0].1.branch_details[0].clone().commits.len(), 1);
 }
 

e2e/playwright/scripts/project-with-commit-and-uncommitted-changes.sh

@@ -0,0 +1,30 @@
+#!/bin/bash
+
+echo "GIT CONFIG $GIT_CONFIG_GLOBAL"
+echo "DATA DIR $GITBUTLER_CLI_DATA_DIR"
+echo "BUT_TESTING $BUT_TESTING"
+
+# Setup a remote project.
+# GitButler currently requires projects to have a remote
+mkdir remote-with-changes
+pushd remote-with-changes
+git init -b master --object-format=sha1
+echo "Initial content" >> initial_file.txt
+git add initial_file.txt
+git commit -am "Initial commit"
+popd
+
+# Clone the remote into a folder
+git clone remote-with-changes local-with-changes
+pushd local-with-changes
+  git checkout master
+
+  # Add an extra commit on the main branch
+  echo "Second commit content" >> second_file.txt
+  git add second_file.txt
+  git commit -am "Second commit on main branch"
+
+  # Add some uncommitted changes
+  echo "Uncommitted changes" >> uncommitted.txt
+  echo "Modified initial file" >> initial_file.txt
+popd

e2e/playwright/tests/addingAProject.spec.ts

@@ -97,3 +97,44 @@ test('should handle gracefully adding a non-git directory', async ({ page, conte
 
 	await waitForTestId(page, 'add-project-not-a-git-repo-modal');
 });
+
+test('should handle adding a project with extra commit and uncommitted changes on main branch', async ({
+	page,
+	context
+}, testInfo) => {
+	const workdir = testInfo.outputPath('workdir');
+	const configdir = testInfo.outputPath('config');
+	gitbutler = await startGitButler(workdir, configdir, context);
+
+	const projectPath = gitbutler.pathInWorkdir('local-with-changes/');
+
+	// Set up repository with extra commit and uncommitted changes
+	await gitbutler.runScript('project-with-commit-and-uncommitted-changes.sh');
+
+	await page.goto('/');
+
+	const onboardingPage = getByTestId(page, 'onboarding-page');
+	await onboardingPage.waitFor();
+
+	clickByTestId(page, 'analytics-continue');
+
+	// Click the add local project button
+	const fileChooserPromise = page.waitForEvent('filechooser');
+	clickByTestId(page, 'add-local-project');
+
+	const fileChooser = await fileChooserPromise;
+	await fileChooser.setFiles(projectPath);
+
+	clickByTestId(page, 'set-base-branch');
+
+	// Should load the workspace directly after setting base branch
+	await waitForTestId(page, 'workspace-view');
+
+	// There should be only one stack
+	const stacks = getByTestId(page, 'stack');
+	await expect(stacks).toHaveCount(1);
+	const stack = stacks.first();
+
+	// The stack should not have the branch called "master"
+	await expect(stack).not.toContainText('master');
+});