refactor(stack sync): Don't pass StackSyncFlags to actions.SyncStack (#…

…136)

Just found this bug while doing some stuff locally.

.mergequeue/config.yml

@@ -6,12 +6,6 @@ merge_rules:
     merge_failed: blocked
   publish_status_check: true
   preconditions:
-    validations:
-    - name: Require conventional commit format
-      match:
-        type: title
-        regex:
-        - '^(fix|feat|chore|docs|style|test|ci|refactor)(\(.+\))?: .+$'
     use_github_mergeability: true
     conversation_resolution_required: true
   merge_mode:

cmd/av/commit_amend.go

@@ -22,7 +22,6 @@ var commitAmendFlags struct {
 	NoEdit bool
 }
 
-
 var commitAmendCmd = &cobra.Command{
 	Use:   "amend",
 	Short: "amend a commit",
@@ -80,7 +79,7 @@ var commitAmendCmd = &cobra.Command{
 			return err
 		}
 
-		err = actions.SyncStack(ctx, repo, client, tx, branchesToSync, state, syncFlags)
+		err = actions.SyncStack(ctx, repo, client, tx, branchesToSync, state)
 		if err != nil {
 			return err
 		}
@@ -90,7 +89,9 @@ var commitAmendCmd = &cobra.Command{
 }
 
 func init() {
-	commitAmendCmd.Flags().StringVarP(&commitAmendFlags.Message, "message", "m", "", "the commit message")
-	commitAmendCmd.Flags().BoolVar(&commitAmendFlags.NoEdit, "no-edit", false, "amend a commit without changing its commit message")
+	commitAmendCmd.Flags().
+		StringVarP(&commitAmendFlags.Message, "message", "m", "", "the commit message")
+	commitAmendCmd.Flags().
+		BoolVar(&commitAmendFlags.NoEdit, "no-edit", false, "amend a commit without changing its commit message")
 	commitAmendCmd.MarkFlagsMutuallyExclusive("message", "no-edit")
-}
+}

cmd/av/commit_create.go

@@ -98,7 +98,7 @@ func commitCreate(repo *git.Repo, currentBranchName string, flags struct {
 		return err
 	}
 
-	err = actions.SyncStack(ctx, repo, client, tx, branchesToSync, state, syncFlags)
+	err = actions.SyncStack(ctx, repo, client, tx, branchesToSync, state)
 	if err != nil {
 		return err
 	}

cmd/av/stack_sync.go

@@ -20,7 +20,14 @@ import (
 	"golang.org/x/exp/slices"
 )
 
-var syncFlags actions.StackSyncFlags
+var stackSyncFlags struct {
+	actions.StackSyncConfig
+
+	All      bool
+	Abort    bool
+	Continue bool
+	Skip     bool
+}
 
 var stackSyncCmd = &cobra.Command{
 	Use:   "sync",
@@ -42,17 +49,6 @@ stack. This is useful for rebasing a whole stack on the latest changes from the
 base branch.
 `),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		// Argument validation
-		if countBools(syncFlags.Continue, syncFlags.Abort, syncFlags.Skip) > 1 {
-			return errors.New("cannot use --continue, --abort, and --skip together")
-		}
-		if syncFlags.Current && syncFlags.Trunk {
-			return errors.New("cannot use --current and --trunk together")
-		}
-		if syncFlags.Parent != "" && syncFlags.Trunk {
-			return errors.New("cannot use --parent and --trunk together")
-		}
-
 		ctx := context.Background()
 
 		repo, err := getRepo()
@@ -73,7 +69,7 @@ base branch.
 			return err
 		}
 
-		if syncFlags.Abort {
+		if stackSyncFlags.Abort {
 			if state.CurrentBranch == "" || state.Continuation == nil {
 				// Try to clear the state file if it exists just to be safe.
 				_ = actions.WriteStackSyncState(repo, nil)
@@ -98,7 +94,7 @@ base branch.
 			return nil
 		}
 
-		if !syncFlags.Skip {
+		if !stackSyncFlags.Skip {
 			// Make sure all changes are staged unless --skip. git rebase --skip will
 			// clean up the changes.
 			diff, err := repo.Diff(&git.DiffOpts{Quiet: true})
@@ -112,7 +108,7 @@ base branch.
 			}
 		}
 
-		if syncFlags.Continue || syncFlags.Skip {
+		if stackSyncFlags.Continue || stackSyncFlags.Skip {
 			if state.CurrentBranch == "" {
 				return errors.New("no sync in progress")
 			}
@@ -136,12 +132,12 @@ base branch.
 
 			state.OriginalBranch = state.CurrentBranch
 			state.Config = actions.StackSyncConfig{
-				Current: syncFlags.Current,
-				Trunk:   syncFlags.Trunk,
-				NoPush:  syncFlags.NoPush,
-				NoFetch: syncFlags.NoFetch,
-				Parent:  syncFlags.Parent,
-				Prune:   syncFlags.Prune,
+				Current: stackSyncFlags.Current,
+				Trunk:   stackSyncFlags.Trunk,
+				NoPush:  stackSyncFlags.NoPush,
+				NoFetch: stackSyncFlags.NoFetch,
+				Parent:  stackSyncFlags.Parent,
+				Prune:   stackSyncFlags.Prune,
 			}
 		}
 
@@ -160,8 +156,8 @@ base branch.
 				NewParent:      state.Config.Parent,
 				NewParentTrunk: state.Config.Parent == defaultBranch,
 			}
-			if syncFlags.Continue || syncFlags.Skip {
-				res, err = actions.ReparentSkipContinue(repo, tx, opts, syncFlags.Skip)
+			if stackSyncFlags.Continue || stackSyncFlags.Skip {
+				res, err = actions.ReparentSkipContinue(repo, tx, opts, stackSyncFlags.Skip)
 			} else {
 				res, err = actions.Reparent(repo, tx, opts)
 			}
@@ -217,7 +213,7 @@ base branch.
 			// we try to do something in the repo before we finish that)
 			branchesToSync = []string{state.CurrentBranch}
 			state.Branches = branchesToSync
-		} else if syncFlags.All {
+		} else if stackSyncFlags.All {
 			for _, br := range tx.AllBranches() {
 				if !br.IsStackRoot() {
 					continue
@@ -256,7 +252,11 @@ base branch.
 			return err
 		}
 
-		err = actions.SyncStack(ctx, repo, client, tx, branchesToSync, state, syncFlags)
+		var syncOpts []actions.SyncStackOpt
+		if stackSyncFlags.Skip {
+			syncOpts = append(syncOpts, actions.WithSkipNextCommit())
+		}
+		err = actions.SyncStack(ctx, repo, client, tx, branchesToSync, state, syncOpts...)
 		if err != nil {
 			return err
 		}
@@ -267,47 +267,51 @@ base branch.
 
 func init() {
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.All, "all", false,
+		&stackSyncFlags.All, "all", false,
 		"synchronize all branches",
 	)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.Current, "current", false,
+		&stackSyncFlags.Current, "current", false,
 		"only sync changes to the current branch\n(don't recurse into descendant branches)",
 	)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.NoPush, "no-push", false,
+		&stackSyncFlags.NoPush, "no-push", false,
 		"do not force-push updated branches to GitHub",
 	)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.NoFetch, "no-fetch", false,
+		&stackSyncFlags.NoFetch, "no-fetch", false,
 		"do not fetch latest PR information from GitHub",
 	)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.Prune, "prune", false,
+		&stackSyncFlags.Prune, "prune", false,
 		"delete the merged branches",
 	)
 	// TODO[mvp]: better name (--to-trunk?)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.Trunk, "trunk", false,
+		&stackSyncFlags.Trunk, "trunk", false,
 		"synchronize the trunk into the stack",
 	)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.Continue, "continue", false,
+		&stackSyncFlags.Continue, "continue", false,
 		"continue an in-progress sync",
 	)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.Abort, "abort", false,
+		&stackSyncFlags.Abort, "abort", false,
 		"abort an in-progress sync",
 	)
 	stackSyncCmd.Flags().BoolVar(
-		&syncFlags.Skip, "skip", false,
+		&stackSyncFlags.Skip, "skip", false,
 		"skip the current commit and continue an in-progress sync",
 	)
 	stackSyncCmd.Flags().StringVar(
-		&syncFlags.Parent, "parent", "",
+		&stackSyncFlags.Parent, "parent", "",
 		"parent branch to rebase onto",
 	)
+
 	stackSyncCmd.MarkFlagsMutuallyExclusive("current", "all")
+	stackSyncCmd.MarkFlagsMutuallyExclusive("current", "trunk")
+	stackSyncCmd.MarkFlagsMutuallyExclusive("trunk", "parent")
+	stackSyncCmd.MarkFlagsMutuallyExclusive("continue", "abort", "skip")
 }
 
 func countBools(bs ...bool) int {

internal/actions/sync_branch.go

@@ -24,7 +24,9 @@ type SyncBranchOpts struct {
 	// If specified, synchronize the branch against the latest version of the
 	// trunk branch. This value is ignored if the branch is not a stack root.
 	ToTrunk bool
-	Skip    bool
+	// If true, skip the current commit.
+	// This must only be set after a rebase conflict in a sync.
+	Skip bool
 
 	Continuation *SyncBranchContinuation
 }

internal/actions/sync_stack.go

@@ -15,7 +15,7 @@ import (
 	"github.com/aviator-co/av/internal/utils/sliceutils"
 )
 
-// stackSyncConfig contains the configuration for a sync operation.
+// StackSyncConfig contains the configuration for a sync operation.
 // It is serializable to JSON to handle the case where the sync is interrupted
 // by a merge conflict (so it can be resumed with the --continue flag).
 type StackSyncConfig struct {
@@ -37,20 +37,7 @@ type StackSyncConfig struct {
 	Prune bool `json:"prune"`
 }
 
-type StackSyncFlags struct {
-	// Include all the options from stackSyncConfig
-	StackSyncConfig
-	// If set, we're continuing a previous sync.
-	Continue bool
-	// If set, abort an in-progress sync operation.
-	Abort bool
-	// If set, skip a commit and continue a previous sync.
-	Skip bool
-	// If set, synchronize all branches.
-	All bool
-}
-
-// stackSyncState is the state of an in-progress sync operation.
+// StackSyncState is the state of an in-progress sync operation.
 // It is written to a file if the sync is interrupted (so it can be resumed with
 // the --continue flag).
 type StackSyncState struct {
@@ -69,17 +56,35 @@ type StackSyncState struct {
 	Config StackSyncConfig `json:"config"`
 }
 
-// Performs stack sync on all branches in branchesToSync.
+type (
+	SyncStackOpt  func(*syncStackOpts)
+	syncStackOpts struct {
+		skipNextCommit bool
+	}
+)
+
+func WithSkipNextCommit() SyncStackOpt {
+	return func(opts *syncStackOpts) {
+		opts.skipNextCommit = true
+	}
+}
+
+// SyncStack performs stack sync on all branches in branchesToSync.
 func SyncStack(ctx context.Context,
 	repo *git.Repo,
 	client *gh.Client,
 	tx meta.WriteTx,
 	branchesToSync []string,
 	state StackSyncState,
-	flags StackSyncFlags,
+	optFns ...SyncStackOpt,
 ) error {
-	state.Branches = branchesToSync
+	opts := &syncStackOpts{}
+	for _, optFn := range optFns {
+		optFn(opts)
+	}
 
+	state.Branches = branchesToSync
+	skip := opts.skipNextCommit
 	for i, currentBranch := range branchesToSync {
 		if i > 0 {
 			// Add spacing in the output between each branch sync
@@ -92,7 +97,7 @@ func SyncStack(ctx context.Context,
 			Push:         !state.Config.NoPush,
 			Continuation: state.Continuation,
 			ToTrunk:      state.Config.Trunk,
-			Skip:         flags.Skip,
+			Skip:         skip,
 		})
 		if err != nil {
 			return err
@@ -108,6 +113,11 @@ func SyncStack(ctx context.Context,
 			return ErrExitSilently{ExitCode: 1}
 		}
 		state.Continuation = nil
+		// If skip was specified, it was because the sync was interrupted by a
+		// conflict. The user wanted to skip a commit and continue the sync. If
+		// we get here, the rebase succeeded, and it doesn't make sense to start
+		// subsequent rebases with `git rebase --skip`.
+		skip = false
 	}
 
 	if state.Config.Prune {