diff --git a/README.md b/README.md index 01689e2..e1b6a83 100644 --- a/README.md +++ b/README.md @@ -545,6 +545,11 @@ so the merged branch goes and the ones above it are restacked onto trunk before otherwise the next PR up would show the merged commits too. A dry run only says which branches that would be. +GitHub won't open a PR for a branch with no commits of its own, so submit stops before pushing +anything when such a branch (without a PR already) sits under another one it is about to submit, +and tells you the `git stack reorder` that moves it to the top (or to pop it, or commit on it). At +the top of what's submitted it's only a notice. + Branches are pushed with `--force-with-lease`, so after a restack your branches are force pushed. If a branch has commits on the remote that aren't in your local one (someone else pushed to it), submit refuses rather than overwrite them. `git sync` pulls them in when the remote is diff --git a/pkg/app/submit.go b/pkg/app/submit.go index d787ade..a580086 100644 --- a/pkg/app/submit.go +++ b/pkg/app/submit.go @@ -11,6 +11,7 @@ import ( "github.com/DomBlack/git-stack/pkg/config" "github.com/DomBlack/git-stack/pkg/forge" "github.com/DomBlack/git-stack/pkg/git" + "github.com/DomBlack/git-stack/pkg/shell" "github.com/DomBlack/git-stack/pkg/stack" ) @@ -192,6 +193,10 @@ func (a *App) submit(ctx context.Context, repo git.Repo, o SubmitOptions) (Submi } } + if err := a.checkEmpty(ctx, repo, s, live(s.Branches), len(live(scope)), before, o.DryRun, &res); err != nil { + return res, err + } + var newBranches []string for _, b := range live(scope) { pr, has := before[b.Name] @@ -767,3 +772,56 @@ func (a *App) closePending(ctx context.Context, repo git.Repo, prs []forge.PullR res.Notices = append(res.Notices, fmt.Sprintf("could not update the list of pull requests to close: %v", err)) } } + +// checkEmpty looks for branches about to be submitted that have no commits +// of their own (everything in them is in the branch below) and no open pull +// request. GitHub won't open a pull request with no commits, so at the top +// that is a notice; under another branch being submitted it stops the +// submit, since the pull requests above have nothing to stack on. The way +// out is to move it to the top, drop it or commit on it. branches are the +// stack's live branches, the first n of them being submitted. A dry run +// only says so. +func (a *App) checkEmpty(ctx context.Context, repo git.Repo, s *stack.Stack, branches []stack.Branch, n int, prs map[string]forge.PullRequest, dryRun bool, res *SubmitResult) error { + var empty []string + parent := s.Trunk + for i, b := range branches[:n] { + pr, has := prs[b.Name] + open := has && (pr.State == forge.StateOpen || pr.State == forge.StateDraft) + count, err := a.d.Git.CountCommits(ctx, repo, parent, b.Name) + parent = b.Name + if err != nil || count > 0 || open { + continue + } + if i == n-1 { + res.Notices = append(res.Notices, fmt.Sprintf("%s has no commits of its own, so GitHub won't open a pull request for it yet", b.Name)) + continue + } + empty = append(empty, b.Name) + } + if len(empty) == 0 { + return nil + } + order := slices.DeleteFunc(s.Names(), func(name string) bool { return slices.Contains(empty, name) }) + order = append(order, empty...) + msg := fmt.Sprintf("%s %s no commits of its own, so GitHub can't open a pull request for %s for the ones above to stack on", + joinNames(empty), pluralise(len(empty), "has", "have"), pluralise(len(empty), "it", "them")) + if dryRun { + res.Notices = append(res.Notices, msg+"; a real submit stops here") + return nil + } + // Branch names can't hold control characters, so quoting never fails. + quote := func(name string) string { + q, _ := shell.Arg(name) + return q + } + args := make([]string, len(order)) + for i, name := range order { + args[i] = quote(name) + } + first := empty[0] + return stack.New(stack.KindInvalidArgs, msg).WithSteps( + fmt.Sprintf("move %s to the top: git stack reorder %s", pluralise(len(empty), "it", "them"), strings.Join(args, " ")), + fmt.Sprintf("or drop %s: git stack checkout %s, then git stack pop", first, quote(first)), + fmt.Sprintf("or commit on %s: git stack checkout %s, then git stack modify", first, quote(first)), + ) +} diff --git a/pkg/app/submit_test.go b/pkg/app/submit_test.go index 457f4d4..5755948 100644 --- a/pkg/app/submit_test.go +++ b/pkg/app/submit_test.go @@ -855,3 +855,38 @@ func kindOf(err error) stack.Kind { } return stack.KindUnknown } + +// An empty branch under another one being submitted stops the submit +// before anything is pushed, saying how to move it to the top; at the top +// it is only a notice. +func TestSubmitRefusesAnEmptyBranchInTheMiddle(t *testing.T) { + deps, sf, _, repo, dir := submitFixture(t) + gittest.Run(t, dir, "branch", "empty", "a") + graph := stack.NewGraph([]stack.Stack{{Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "empty"}, {Name: "b"}}}}) + deps.Meta = memMeta{graph} + sf.graph = graph + + res, err := app.New(deps).Submit(context.Background(), repo, app.SubmitOptions{DryRun: true}) + if err != nil || !slices.ContainsFunc(res.Notices, func(n string) bool { return strings.Contains(n, "empty has no commits of its own") }) { + t.Errorf("dry run: %v %q", err, res.Notices) + } + + _, err = app.New(deps).Submit(context.Background(), repo, app.SubmitOptions{NoEdit: true}) + se, ok := errors.AsType[*stack.Error](err) + if !ok || se.Kind != stack.KindInvalidArgs || !slices.Contains(se.NextSteps, "move it to the top: git stack reorder a b empty") { + t.Fatalf("err = %#v", err) + } + if len(sf.opts) != 0 { + t.Errorf("nothing may be submitted: %+v", sf.opts) + } + + // At the top it's only a notice. + graph = stack.NewGraph([]stack.Stack{{Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}, {Name: "empty"}}}}) + deps.Meta = memMeta{graph} + sf.graph = graph + gittest.Run(t, dir, "branch", "-f", "empty", "b") + res, err = app.New(deps).Submit(context.Background(), repo, app.SubmitOptions{NoEdit: true}) + if err != nil || !slices.ContainsFunc(res.Notices, func(n string) bool { return strings.Contains(n, "won't open a pull request for it yet") }) { + t.Errorf("top: %v %q", err, res.Notices) + } +}