From b715e022c6a67d219999218b59f473a66265923f Mon Sep 17 00:00:00 2001 From: Edmund Kohlwey Date: Sat, 20 Jun 2026 07:54:05 -0400 Subject: [PATCH] submit: protect co-committer commits with a proper force-with-lease Lease against the commit git-spice last pushed (UpstreamLastPushedHash) instead of the local remote-tracking ref. The tracking ref advances on every fetch, silently defeating --force-with-lease and letting submit overwrite commits pushed by someone else. Submit now classifies the remote against our last-pushed baseline and refuses to overwrite when the remote has advanced (listing the foreign commits and pointing to 'gs branch sync') or diverged. Because the baseline is the remote-side last-pushed hash, this survives restacks, which only rewrite local hashes. Also carry the baseline when 'branch split' reassigns a CR to a new branch, and record it after a conflicted 'branch sync --rebase' is resumed, so a clean (non-force) submit works in both cases. --- .../unreleased/Fixed-20260620-071248.yaml | 3 + branch_sync.go | 23 ++ doc/src/guide/cr.md | 12 ++ internal/git/integration_test.go | 18 +- internal/git/rev_list.go | 24 ++- internal/handler/branchsync/handler.go | 36 +++- internal/handler/split/handler.go | 19 +- internal/handler/submit/errors.go | 89 ++++++++ internal/handler/submit/handler.go | 53 +++-- internal/handler/submit/mocks_test.go | 39 ++++ internal/handler/submit/reconcile.go | 200 ++++++++++++++++++ internal/handler/submit/reconcile_test.go | 106 ++++++++++ ...h_submit_detect_existing_upstream_name.txt | 3 +- testdata/script/branch_submit_force_push.txt | 6 +- .../script/branch_submit_remote_advanced.txt | 86 ++++++++ .../script/branch_submit_remote_diverged.txt | 67 ++++++ 16 files changed, 749 insertions(+), 35 deletions(-) create mode 100644 .changes/unreleased/Fixed-20260620-071248.yaml create mode 100644 internal/handler/submit/errors.go create mode 100644 internal/handler/submit/reconcile.go create mode 100644 internal/handler/submit/reconcile_test.go create mode 100644 testdata/script/branch_submit_remote_advanced.txt create mode 100644 testdata/script/branch_submit_remote_diverged.txt diff --git a/.changes/unreleased/Fixed-20260620-071248.yaml b/.changes/unreleased/Fixed-20260620-071248.yaml new file mode 100644 index 000000000..3429faee4 --- /dev/null +++ b/.changes/unreleased/Fixed-20260620-071248.yaml @@ -0,0 +1,3 @@ +kind: Fixed +body: 'submit: Refuse to overwrite commits added to a branch by someone else since git-spice last pushed it; the offending commits are listed. Run ''gs branch sync'' to integrate them, or use --force to overwrite.' +time: 2026-06-20T07:12:48.071369-04:00 diff --git a/branch_sync.go b/branch_sync.go index c8f20169e..a96000ec8 100644 --- a/branch_sync.go +++ b/branch_sync.go @@ -7,12 +7,18 @@ import ( "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/branchsync" "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" "go.abhg.dev/gs/internal/text" ) type branchSyncCmd struct { Branch string `placeholder:"NAME" help:"Branch to sync" predictor:"trackedBranches"` Rebase bool `help:"On divergence (both local and remote have new commits since the last push), replay remote-side commits onto local. Conflicts surface as a normal interrupted rebase; resume with 'gs rebase --continue'."` + + // RecordPushed is set only by the rebase continuation that finishes a + // conflicted '--rebase' sync: it records the integrated remote head as + // the last-pushed baseline without re-running the rebase. + RecordPushed string `hidden:"" help:"Record the given hash as the last-pushed baseline and exit."` } func (*branchSyncCmd) Help() string { @@ -45,14 +51,31 @@ func (cmd *branchSyncCmd) AfterApply(ctx context.Context, wt *git.Worktree) erro func (cmd *branchSyncCmd) Run( ctx context.Context, log *silog.Logger, + svc *spice.Service, handler *branchsync.Handler, ) error { req := branchsync.SyncRequest{Branch: cmd.Branch} if cmd.Rebase { req.Mode = branchsync.ModeRebase } + if cmd.RecordPushed != "" { + req.RecordPushed = git.Hash(cmd.RecordPushed) + } + res, err := handler.Sync(ctx, req) if err != nil { + // A '--rebase' sync can be interrupted by a conflict. Once the user + // resolves it and runs 'gs rebase continue', re-running the rebase + // would conflict again, so the continuation only records the + // integrated remote head (res.ToHash) as the last-pushed baseline. + if res != nil && !res.ToHash.IsZero() { + return svc.RebaseRescue(ctx, spice.RebaseRescueRequest{ + Err: err, + Command: []string{"branch", "sync", "--record-pushed", res.ToHash.String()}, + Branch: cmd.Branch, + Message: "interrupted: sync branch " + cmd.Branch, + }) + } return err } diff --git a/doc/src/guide/cr.md b/doc/src/guide/cr.md index 500947076..35326024c 100644 --- a/doc/src/guide/cr.md +++ b/doc/src/guide/cr.md @@ -191,6 +191,18 @@ if the operation could result in data loss. To override these safety checks and push to a branch anyway, use the `--force` flag. + + +In particular, if someone else has pushed commits to a branch +since git-spice last pushed it +(for example, a maintainer adding changes to your CR), +submitting will stop and list those commits +instead of overwriting them. +Use $$gs branch sync$$ to bring the commits into your branch, +then submit again. +This protection is based on the commit git-spice last pushed, +so it holds even after the branch has been restacked. + ### Update existing CRs only diff --git a/internal/git/integration_test.go b/internal/git/integration_test.go index 6d0078efb..41681c6a5 100644 --- a/internal/git/integration_test.go +++ b/internal/git/integration_test.go @@ -70,16 +70,18 @@ func TestIntegrationCommitListing(t *testing.T) { assert.Equal(t, []git.CommitDetail{ { - Hash: "9045e63b1d4d4e6e2db3fa03d4eb98a6ed653f3a", - ShortHash: "9045e63", - Subject: "Add feature2", - AuthorDate: time.Date(2024, 8, 27, 22, 10, 11, 0, time.UTC), + Hash: "9045e63b1d4d4e6e2db3fa03d4eb98a6ed653f3a", + ShortHash: "9045e63", + Subject: "Add feature2", + AuthorEmail: "test@example.com", + AuthorDate: time.Date(2024, 8, 27, 22, 10, 11, 0, time.UTC), }, { - Hash: "acca66548dc31594dc3bc669c804e98eda1edc3d", - ShortHash: "acca665", - Subject: "Add feature1", - AuthorDate: time.Date(2024, 8, 27, 21, 52, 12, 0, time.UTC), + Hash: "acca66548dc31594dc3bc669c804e98eda1edc3d", + ShortHash: "acca665", + Subject: "Add feature1", + AuthorEmail: "test@example.com", + AuthorDate: time.Date(2024, 8, 27, 21, 52, 12, 0, time.UTC), }, }, commits) }) diff --git a/internal/git/rev_list.go b/internal/git/rev_list.go index edffeea9f..8fb74c737 100644 --- a/internal/git/rev_list.go +++ b/internal/git/rev_list.go @@ -37,6 +37,9 @@ type CommitDetail struct { // Subject is the first line of the commit message. Subject string + // AuthorEmail is the email address of the commit author. + AuthorEmail string + // AuthorDate is the time the commit was authored. AuthorDate time.Time } @@ -48,7 +51,9 @@ func (cd *CommitDetail) String() string { // ListCommitsDetails returns details about commits matched by the given range. func (r *Repository) ListCommitsDetails(ctx context.Context, commits CommitRange) iter.Seq2[CommitDetail, error] { return func(yield func(CommitDetail, error) bool) { - for line, err := range r.listCommitsFormat(ctx, commits, "%H %h %at %s") { + // %ae (author email) carries no spaces, so it stays a single + // space-delimited field ahead of the free-form subject. + for line, err := range r.listCommitsFormat(ctx, commits, "%H %h %at %ae %s") { if err != nil { yield(CommitDetail{}, err) return @@ -66,7 +71,7 @@ func (r *Repository) ListCommitsDetails(ctx context.Context, commits CommitRange continue } - epochstr, subject, ok := strings.Cut(line, " ") + epochstr, line, ok := strings.Cut(line, " ") if !ok { r.log.Warn("Bad rev-list output", "line", line, "error", "missing an time") continue @@ -77,11 +82,18 @@ func (r *Repository) ListCommitsDetails(ctx context.Context, commits CommitRange continue } + authorEmail, subject, ok := strings.Cut(line, " ") + if !ok { + r.log.Warn("Bad rev-list output", "line", line, "error", "missing an author email") + continue + } + if !yield(CommitDetail{ - Hash: Hash(hash), - ShortHash: Hash(shortHash), - Subject: subject, - AuthorDate: time.Unix(epoch, 0), + Hash: Hash(hash), + ShortHash: Hash(shortHash), + Subject: subject, + AuthorEmail: authorEmail, + AuthorDate: time.Unix(epoch, 0), }, nil) { return } diff --git a/internal/handler/branchsync/handler.go b/internal/handler/branchsync/handler.go index bf60ffed9..87f62cf6e 100644 --- a/internal/handler/branchsync/handler.go +++ b/internal/handler/branchsync/handler.go @@ -125,6 +125,16 @@ const ( type SyncRequest struct { Branch string Mode Mode + + // RecordPushed, when set, makes Sync skip all fetching and rebasing and + // only record the given hash as the branch's last-pushed baseline. + // + // It is used to finish a '--rebase' sync that was interrupted by a + // conflict: once the user resolves it and runs 'gs rebase continue', the + // remote commits are integrated, but the baseline still needs to be + // recorded. Re-running the full sync would try to rebase again, so the + // continuation records the baseline directly instead. + RecordPushed git.Hash } // Sync syncs a single tracked branch according to the request. Returns @@ -138,6 +148,26 @@ func (h *Handler) Sync(ctx context.Context, req SyncRequest) (*SyncResult, error return &SyncResult{Branch: branch, Action: ActionSkipped, SkipReason: "trunk is synced by 'gs repo sync'"}, nil } + // Finish an interrupted '--rebase' sync: just record the baseline. + if !req.RecordPushed.IsZero() { + lookup, err := h.Store.LookupBranch(ctx, branch) + if err != nil { + return nil, fmt.Errorf("lookup branch: %w", err) + } + if lookup.UpstreamBranch == "" { + return nil, ErrNoUpstream + } + if err := h.recordPushedHash(ctx, branch, lookup.UpstreamBranch, req.RecordPushed); err != nil { + return nil, err + } + return &SyncResult{ + Branch: branch, + Action: ActionRebased, + FromHash: req.RecordPushed, + ToHash: req.RecordPushed, + }, nil + } + lookup, err := h.Store.LookupBranch(ctx, branch) if err != nil { return nil, fmt.Errorf("lookup branch: %w", err) @@ -243,7 +273,11 @@ func (h *Handler) Sync(ctx context.Context, req SyncRequest) (*SyncResult, error // already guarantees. This is what lets remote-side commits survive // even after the branch has been restacked locally. if err := h.rebase(ctx, branch, lookup.UpstreamBranch, localHash, remoteHash, pHash); err != nil { - return nil, err + // The rebase was interrupted (typically a conflict). Surface the + // integration target so the caller can record it as the baseline + // once the user resolves the conflict and resumes. + res.ToHash = remoteHash + return res, err } if err := h.recordPushedHash(ctx, branch, lookup.UpstreamBranch, remoteHash); err != nil { log.Warn("Could not record pushed hash", "error", err) diff --git a/internal/handler/split/handler.go b/internal/handler/split/handler.go index d0f34f32f..75080bf49 100644 --- a/internal/handler/split/handler.go +++ b/internal/handler/split/handler.go @@ -37,6 +37,7 @@ type Store interface { Trunk() string Remote() (state.Remote, error) BeginBranchTx() *state.BranchTx + LookupBranch(ctx context.Context, name string) (*state.LookupResponse, error) } var _ Store = (*state.Store)(nil) @@ -354,6 +355,15 @@ func (h *Handler) prepareChangeMetadataTransfer( toUpstreamBranch := cmp.Or(upstreamBranch, fromBranch) + // Carry the last-pushed baseline along with the upstream branch, so a + // later 'submit' of toBranch can still tell whether the remote has been + // touched by someone else. Without it, toBranch would have no baseline + // and submit would refuse to push. + var lastPushed *git.Hash + if resp, err := h.Store.LookupBranch(ctx, fromBranch); err == nil && !resp.UpstreamLastPushedHash.IsZero() { + lastPushed = &resp.UpstreamLastPushedHash + } + var empty string if err := tx.Upsert(ctx, state.UpsertRequest{ Name: fromBranch, @@ -364,10 +374,11 @@ func (h *Handler) prepareChangeMetadataTransfer( } if err := tx.Upsert(ctx, state.UpsertRequest{ - Name: toBranch, - ChangeMetadata: metaJSON, - ChangeForge: forgeID, - UpstreamBranch: &toUpstreamBranch, + Name: toBranch, + ChangeMetadata: metaJSON, + ChangeForge: forgeID, + UpstreamBranch: &toUpstreamBranch, + UpstreamLastPushedHash: lastPushed, }); err != nil { return nil, fmt.Errorf("set change metadata on %v: %w", toBranch, err) } diff --git a/internal/handler/submit/errors.go b/internal/handler/submit/errors.go new file mode 100644 index 000000000..ee35b5e37 --- /dev/null +++ b/internal/handler/submit/errors.go @@ -0,0 +1,89 @@ +package submit + +import ( + "fmt" + + "go.abhg.dev/gs/internal/git" +) + +// RemoteAdvancedError indicates that the remote upstream branch gained +// commits on top of what git-spice last pushed, so a force-push would +// discard them. +// +// The user can bring those commits into their branch with 'gs branch sync' +// and submit again, or use --force to overwrite them. +type RemoteAdvancedError struct { + // Branch is the local branch being submitted. + Branch string + + // Remote is the name of the remote being pushed to. + Remote string + + // UpstreamBranch is the name of the branch on the remote. + UpstreamBranch string + + // Commits are the commits present on the remote + // that are not in the local branch, newest first. + Commits []git.CommitDetail +} + +func (e *RemoteAdvancedError) Error() string { + return fmt.Sprintf( + "%v/%v has %d commit(s) not in branch %v", + e.Remote, e.UpstreamBranch, len(e.Commits), e.Branch, + ) +} + +// RemoteDivergedError indicates that the remote upstream branch was rewritten +// away from what git-spice last pushed, so we cannot identify what a +// force-push would discard. +type RemoteDivergedError struct { + // Branch is the local branch being submitted. + Branch string + + // Remote is the name of the remote being pushed to. + Remote string + + // UpstreamBranch is the name of the branch on the remote. + UpstreamBranch string + + // LastPushed is the commit git-spice last pushed to the upstream branch. + LastPushed git.Hash + + // RemoteHash is the upstream branch's current head. + RemoteHash git.Hash +} + +func (e *RemoteDivergedError) Error() string { + return fmt.Sprintf( + "%v/%v was rewritten away from the last pushed commit %v", + e.Remote, e.UpstreamBranch, e.LastPushed.Short(), + ) +} + +// RemoteNoBaselineError indicates that git-spice has no record of what it last +// pushed to the upstream branch, and the remote differs from the local branch, +// so it cannot prove that a force-push is safe. +type RemoteNoBaselineError struct { + // Branch is the local branch being submitted. + Branch string + + // Remote is the name of the remote being pushed to. + Remote string + + // UpstreamBranch is the name of the branch on the remote. + UpstreamBranch string + + // RemoteHash is the upstream branch's current head. + RemoteHash git.Hash + + // LocalHash is the local branch's head. + LocalHash git.Hash +} + +func (e *RemoteNoBaselineError) Error() string { + return fmt.Sprintf( + "no record of the last commit pushed to %v/%v", + e.Remote, e.UpstreamBranch, + ) +} diff --git a/internal/handler/submit/handler.go b/internal/handler/submit/handler.go index 8b4568dcb..7b8c4f74e 100644 --- a/internal/handler/submit/handler.go +++ b/internal/handler/submit/handler.go @@ -8,6 +8,7 @@ import ( "encoding" "errors" "fmt" + "iter" "os" "slices" "sort" @@ -38,6 +39,9 @@ type GitRepository interface { Var(ctx context.Context, name string) (string, error) CommitMessageRange(ctx context.Context, start string, stop string) ([]git.CommitMessage, error) RemoteFetchRefspecs(ctx context.Context, remote string) ([]git.Refspec, error) + IsAncestor(ctx context.Context, a, b git.Hash) bool + Fetch(ctx context.Context, opts git.FetchOptions) error + ListCommitsDetails(ctx context.Context, commits git.CommitRange) iter.Seq2[git.CommitDetail, error] } var _ GitRepository = (*git.Repository)(nil) @@ -56,6 +60,7 @@ type Store interface { BeginBranchTx() *state.BranchTx Trunk() string + LookupBranch(ctx context.Context, name string) (*state.LookupResponse, error) LoadPreparedBranch(ctx context.Context, name string) (*state.PreparedBranch, error) SavePreparedBranch(ctx context.Context, b *state.PreparedBranch) error ClearPreparedBranch(ctx context.Context, name string) error @@ -650,6 +655,14 @@ func (h *Handler) submitBranch( return status, fmt.Errorf("peel to commit: %w", err) } + // lastPushed is the commit git-spice last recorded pushing to the + // upstream branch. It is the baseline for the --force-with-lease used + // below: we refuse to force-push if the remote has moved past it. + var lastPushed git.Hash + if resp, err := h.Store.LookupBranch(ctx, branchToSubmit); err == nil { + lastPushed = resp.UpstreamLastPushedHash + } + remote, err := h.remote(ctx) if err != nil { return status, fmt.Errorf("get remote: %w", err) @@ -982,15 +995,25 @@ func (h *Handler) submitBranch( NoVerify: opts.NoVerify, } - // If we've already pushed this branch before, - // we'll need a force push. - // Use a --force-with-lease to avoid - // overwriting someone else's changes. + // If we've already pushed this branch before, we'll need a force + // push. Use a --force-with-lease against the commit we last pushed + // so we don't overwrite commits added by someone else. if !opts.Force { - existingHash, err := h.Repository.PeelToCommit(ctx, remote.Push+"/"+upstreamBranch) - if err == nil { - pushOpts.ForceWithLease = upstreamBranch + ":" + existingHash.String() + // No CR is associated yet, so there is no forge head to + // consult; fall back to the remote-tracking ref, which is + // zero if the branch does not exist on the remote. + var remoteHead git.Hash + if h, err := h.Repository.PeelToCommit(ctx, remote.Push+"/"+upstreamBranch); err == nil { + remoteHead = h } + + lease, err := h.ensureSafePush(ctx, log, + branchToSubmit, remote.Push, upstreamBranch, + lastPushed, remoteHead, commitHash) + if err != nil { + return status, err + } + pushOpts.ForceWithLease = lease } err = h.Worktree.Push(ctx, pushOpts) @@ -1179,16 +1202,20 @@ func (h *Handler) submitBranch( NoVerify: opts.NoVerify, } if !opts.Force { - // Force push, but only if the ref is exactly - // where we think it is. - existingHash, err := h.Repository.PeelToCommit(ctx, remote.Push+"/"+upstreamBranch) - if err == nil { - pushOpts.ForceWithLease = upstreamBranch + ":" + existingHash.String() + // Force push, but only if the remote is still where we + // last left it. The forge already told us its current head + // (pull.HeadHash), so this needs no extra fetch. + lease, err := h.ensureSafePush(ctx, log, + branchToSubmit, remote.Push, upstreamBranch, + lastPushed, pull.HeadHash, commitHash) + if err != nil { + return status, err } + pushOpts.ForceWithLease = lease } if err := h.Worktree.Push(ctx, pushOpts); err != nil { - log.Error("Push failed. Branch may have been updated by someone else. Try with --force.") + log.Error("Push failed. The remote may have changed since we checked. Run 'gs branch sync', then submit again.") return status, fmt.Errorf("push branch: %w", err) } diff --git a/internal/handler/submit/mocks_test.go b/internal/handler/submit/mocks_test.go index 058e1bfcc..0939eaef1 100644 --- a/internal/handler/submit/mocks_test.go +++ b/internal/handler/submit/mocks_test.go @@ -375,6 +375,45 @@ func (c *MockStoreLoadPreparedBranchCall) DoAndReturn(f func(context.Context, st return c } +// LookupBranch mocks base method. +func (m *MockStore) LookupBranch(ctx context.Context, name string) (*state.LookupResponse, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LookupBranch", ctx, name) + ret0, _ := ret[0].(*state.LookupResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// LookupBranch indicates an expected call of LookupBranch. +func (mr *MockStoreMockRecorder) LookupBranch(ctx, name any) *MockStoreLookupBranchCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LookupBranch", reflect.TypeOf((*MockStore)(nil).LookupBranch), ctx, name) + return &MockStoreLookupBranchCall{Call: call} +} + +// MockStoreLookupBranchCall wrap *gomock.Call +type MockStoreLookupBranchCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockStoreLookupBranchCall) Return(arg0 *state.LookupResponse, arg1 error) *MockStoreLookupBranchCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockStoreLookupBranchCall) Do(f func(context.Context, string) (*state.LookupResponse, error)) *MockStoreLookupBranchCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockStoreLookupBranchCall) DoAndReturn(f func(context.Context, string) (*state.LookupResponse, error)) *MockStoreLookupBranchCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + // SavePreparedBranch mocks base method. func (m *MockStore) SavePreparedBranch(ctx context.Context, b *state.PreparedBranch) error { m.ctrl.T.Helper() diff --git a/internal/handler/submit/reconcile.go b/internal/handler/submit/reconcile.go new file mode 100644 index 000000000..3edfea766 --- /dev/null +++ b/internal/handler/submit/reconcile.go @@ -0,0 +1,200 @@ +package submit + +import ( + "context" + + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/sliceutil" +) + +// remoteDecision categorizes the relationship between the remote upstream +// branch and the commit git-spice is about to push to it. +// +// It is the basis for deciding whether a force-push is safe: +// a force-push must never discard commits git-spice did not push itself. +type remoteDecision int + +const ( + // decisionSafe means the remote branch is exactly where git-spice last + // left it, or does not exist yet. Force-pushing cannot destroy commits + // we did not push. + decisionSafe remoteDecision = iota + + // decisionFastForward means the local branch already contains every + // commit on the remote, so pushing is an ordinary fast-forward that + // needs neither --force nor a lease. + decisionFastForward + + // decisionUpToDate means the remote already points at the commit we + // want to push; there is nothing to do. + decisionUpToDate + + // decisionNoBaseline means git-spice has no record of what it last + // pushed and the remote differs from the local branch, + // so it cannot prove the push is safe. + decisionNoBaseline + + // decisionAdvanced means the remote gained commits on top of our last + // push: lastPushed..remoteHead is a non-empty, well-formed range. + // Force-pushing would discard those commits. + decisionAdvanced + + // decisionDiverged means the remote was rewritten away from our last + // push: lastPushed is not an ancestor of remoteHead. + // We cannot identify what a force-push would discard. + decisionDiverged +) + +// classifyRemote decides whether it is safe to force-push local +// over the remote upstream branch. +// +// - lastPushed is the commit git-spice last recorded pushing to the +// upstream branch, or the zero hash if it was never recorded. +// - remoteHead is the upstream branch's current head, or the zero hash +// if the branch does not exist on the remote. +// - local is the commit we want to push. +// +// isAncestor reports whether a is an ancestor of b. +// +// The lease baseline is lastPushed, not the local remote-tracking ref: +// a tracking ref advances on every 'git fetch', which would silently make +// a --force-with-lease pass even after someone else pushed. lastPushed only +// changes when git-spice itself pushes, so it survives restacks (which only +// rewrite local hashes) while still catching commits added by others. +func classifyRemote( + lastPushed, remoteHead, local git.Hash, + isAncestor func(a, b git.Hash) bool, +) remoteDecision { + switch { + case remoteHead.IsZero(): + return decisionSafe + case remoteHead == local: + return decisionUpToDate + case isAncestor(remoteHead, local): + // Local already contains every remote commit, so pushing only + // adds commits on top: an ordinary fast-forward. This is safe + // even without a recorded baseline, and git itself rejects it + // if the remote moves out from under us before the push lands. + return decisionFastForward + case lastPushed.IsZero(): + return decisionNoBaseline + case remoteHead == lastPushed: + return decisionSafe + case isAncestor(lastPushed, remoteHead): + return decisionAdvanced + default: + return decisionDiverged + } +} + +// ensureSafePush verifies that force-pushing local to the remote upstream +// branch will not discard commits git-spice did not push. +// +// It returns the --force-with-lease value to use when pushing +// (empty if no lease is needed, e.g. a first push), or a typed error +// describing why the push must not proceed. On a blocking decision, it logs +// actionable guidance before returning the error. +// +// remoteHead is the upstream branch's current head, or the zero hash if the +// branch does not exist on the remote. lastPushed is the commit git-spice +// last recorded pushing to the upstream branch (zero if never recorded). +func (h *Handler) ensureSafePush( + ctx context.Context, + log *silog.Logger, + branch, remoteName, upstreamBranch string, + lastPushed, remoteHead, local git.Hash, +) (lease string, err error) { + // Classifying the push relies on comparing commits with merge-base, + // so the remote head must exist locally. The forge hands us its hash + // but not the object, so fetch it when it isn't already present + // (e.g. someone else pushed since our last fetch). + if !remoteHead.IsZero() && remoteHead != local { + if _, err := h.Repository.PeelToCommit(ctx, remoteHead.String()); err != nil { + if ferr := h.Repository.Fetch(ctx, git.FetchOptions{ + Remote: remoteName, + Refspecs: []git.Refspec{git.Refspec(upstreamBranch)}, + }); ferr != nil { + log.Warn("Could not fetch remote branch", "error", ferr) + } + } + } + + switch classifyRemote(lastPushed, remoteHead, local, func(a, b git.Hash) bool { + return h.Repository.IsAncestor(ctx, a, b) + }) { + case decisionSafe: + // A first push (remoteHead absent) needs no lease. + // Otherwise the remote sits exactly at our last push, so lease + // against it: the push is rejected if anyone moved it since. + if remoteHead.IsZero() { + return "", nil + } + return upstreamBranch + ":" + lastPushed.String(), nil + + case decisionFastForward, decisionUpToDate: + return "", nil + + case decisionNoBaseline: + log.Errorf("%v: Not pushing: no record of what git-spice last pushed to %v/%v, and it differs from your branch.", + branch, remoteName, upstreamBranch) + log.Errorf("Run 'gs branch sync' to adopt the remote as a baseline, or use --force to overwrite it.") + return "", &RemoteNoBaselineError{ + Branch: branch, + Remote: remoteName, + UpstreamBranch: upstreamBranch, + RemoteHash: remoteHead, + LocalHash: local, + } + + case decisionAdvanced: + commits := h.foreignCommits(ctx, log, lastPushed, remoteHead) + log.Errorf("%v: Not pushing: %v/%v has commits that are not in your branch:", + branch, remoteName, upstreamBranch) + for _, c := range commits { + log.Errorf(" %v %v (%v)", c.ShortHash, c.Subject, c.AuthorEmail) + } + log.Errorf("Someone may have pushed to this branch.") + log.Errorf("Run 'gs branch sync' to bring those commits into your branch, then submit again.") + log.Errorf("To overwrite them anyway, use --force.") + return "", &RemoteAdvancedError{ + Branch: branch, + Remote: remoteName, + UpstreamBranch: upstreamBranch, + Commits: commits, + } + + default: // decisionDiverged + log.Errorf("%v: Not pushing: %v/%v was rewritten and no longer contains your last push (%v).", + branch, remoteName, upstreamBranch, lastPushed.Short()) + log.Errorf("It now points at %v. Inspect the remote branch; use --force to overwrite it.", + remoteHead.Short()) + return "", &RemoteDivergedError{ + Branch: branch, + Remote: remoteName, + UpstreamBranch: upstreamBranch, + LastPushed: lastPushed, + RemoteHash: remoteHead, + } + } +} + +// foreignCommits lists the commits present on the remote upstream branch +// (lastPushed..remoteHead) that are not in the local branch, newest first. +// +// ensureSafePush has already fetched the remote head, so the commits are +// available locally. Failures are logged and yield a nil slice: the caller +// still blocks the push, it just can't enumerate the offending commits. +func (h *Handler) foreignCommits( + ctx context.Context, + log *silog.Logger, + lastPushed, remoteHead git.Hash, +) []git.CommitDetail { + commits, err := sliceutil.CollectErr(h.Repository.ListCommitsDetails(ctx, + git.CommitRangeFrom(remoteHead).ExcludeFrom(lastPushed))) + if err != nil { + log.Warn("Could not list new remote commits", "error", err) + return nil + } + return commits +} diff --git a/internal/handler/submit/reconcile_test.go b/internal/handler/submit/reconcile_test.go new file mode 100644 index 000000000..4e38578c9 --- /dev/null +++ b/internal/handler/submit/reconcile_test.go @@ -0,0 +1,106 @@ +package submit + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.abhg.dev/gs/internal/git" +) + +func TestClassifyRemote(t *testing.T) { + const ( + lastPushed = git.Hash("1111111111111111111111111111111111111111") + remoteHead = git.Hash("2222222222222222222222222222222222222222") + local = git.Hash("3333333333333333333333333333333333333333") + ) + + tests := []struct { + name string + + lastPushed git.Hash + remoteHead git.Hash + local git.Hash + + // ffAncestor is the result of isAncestor(remoteHead, local): + // whether the push would be a fast-forward. + ffAncestor bool + + // advAncestor is the result of isAncestor(lastPushed, remoteHead): + // whether the remote advanced from our last push. + advAncestor bool + + want remoteDecision + }{ + { + name: "RemoteAbsent", + lastPushed: lastPushed, + remoteHead: git.ZeroHash, + local: local, + want: decisionSafe, + }, + { + name: "UpToDate", + lastPushed: lastPushed, + remoteHead: local, + local: local, + want: decisionUpToDate, + }, + { + name: "FastForward", + lastPushed: lastPushed, + remoteHead: remoteHead, + local: local, + ffAncestor: true, + want: decisionFastForward, + }, + { + name: "NoBaseline", + lastPushed: git.ZeroHash, + remoteHead: remoteHead, + local: local, + want: decisionNoBaseline, + }, + { + name: "RemoteUnchangedAfterRestack", + lastPushed: lastPushed, + remoteHead: lastPushed, + local: local, + want: decisionSafe, + }, + { + name: "Advanced", + lastPushed: lastPushed, + remoteHead: remoteHead, + local: local, + advAncestor: true, + want: decisionAdvanced, + }, + { + name: "Diverged", + lastPushed: lastPushed, + remoteHead: remoteHead, + local: local, + want: decisionDiverged, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := classifyRemote( + tt.lastPushed, tt.remoteHead, tt.local, + func(a, b git.Hash) bool { + switch { + case a == tt.remoteHead && b == tt.local: + return tt.ffAncestor + case a == tt.lastPushed && b == tt.remoteHead: + return tt.advAncestor + default: + t.Fatalf("unexpected isAncestor(%v, %v)", a, b) + return false + } + }, + ) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/testdata/script/branch_submit_detect_existing_upstream_name.txt b/testdata/script/branch_submit_detect_existing_upstream_name.txt index 4c3c7abc5..0f103f76d 100644 --- a/testdata/script/branch_submit_detect_existing_upstream_name.txt +++ b/testdata/script/branch_submit_detect_existing_upstream_name.txt @@ -47,7 +47,8 @@ feature 1 "hash": "2ff547329e55465505b962b0615d398764bea3de" }, "upstream": { - "branch": "feature1" + "branch": "feature1", + "lastPushedHash": "93a14f446ab82c8c39f0c233a8a0a0f047ec4761" }, "change": { "shamhub": { diff --git a/testdata/script/branch_submit_force_push.txt b/testdata/script/branch_submit_force_push.txt index 530795ecb..979cef8ce 100644 --- a/testdata/script/branch_submit_force_push.txt +++ b/testdata/script/branch_submit_force_push.txt @@ -37,9 +37,11 @@ cp $WORK/extra/feature1-new.txt feature1.txt git add feature1.txt git commit -m 'Update feature1' +# Submit refuses to overwrite the commit pushed from elsewhere. ! gs branch submit -stderr 'Branch may have been updated by someone else' -stderr 'failed to push some refs' +stderr 'has commits that are not in your branch' +stderr 'Introduce a conflict' +stderr 'use --force' gs branch submit --force diff --git a/testdata/script/branch_submit_remote_advanced.txt b/testdata/script/branch_submit_remote_advanced.txt new file mode 100644 index 000000000..8342a6587 --- /dev/null +++ b/testdata/script/branch_submit_remote_advanced.txt @@ -0,0 +1,86 @@ +# 'gs branch submit' refuses to overwrite commits that someone else pushed +# to the branch since our last push, and points to 'gs branch sync' to bring +# them in. After syncing, submitting succeeds and the other commit survives. + +as 'Test ' +at '2026-06-19T10:00:00Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create feat1 and submit it +git add feat1.txt +gs bc -m 'Add feat1' feat1 +gs branch submit --fill +stderr 'Created #' + +# A maintainer pushes a commit on top of feat1 on the remote. +cd .. +git clone $SHAMHUB_URL/alice/example.git fork +cd fork +git checkout feat1 +cp $WORK/extra/helper.txt helper.txt +git add helper.txt +git commit -m 'Add helper from maintainer' +git push origin feat1 + +# Meanwhile we make a local change and try to submit. +cd ../repo +cp $WORK/extra/feat1-edit.txt feat1.txt +git add feat1.txt +git commit -m 'Edit feat1 locally' + +# Submit refuses: it will not discard the maintainer's commit, and it +# surfaces the offending commit and the recommended recovery. +! gs branch submit +stderr 'has commits that are not in your branch' +stderr 'Add helper from maintainer' +stderr 'gs branch sync' +stderr 'use --force' + +# The remote still has the maintainer's commit; nothing was overwritten. +cd ../fork +git fetch +git log --pretty=%s origin/feat1 +cmp stdout $WORK/golden/remote-before-sync.txt + +# Bring the maintainer's commit into our branch and submit again. +cd ../repo +gs branch sync --rebase +stderr 'feat1: rebased' + +gs branch submit +stderr 'Updated #' + +# Both the maintainer's helper and our edit are now on the remote. +cd ../fork +git fetch +git log --pretty=%s origin/feat1 +cmp stdout $WORK/golden/remote-after-sync.txt + +-- repo/feat1.txt -- +feat 1 +-- extra/feat1-edit.txt -- +feat 1 +edited locally +-- extra/helper.txt -- +helper from maintainer +-- golden/remote-before-sync.txt -- +Add helper from maintainer +Add feat1 +Initial commit +-- golden/remote-after-sync.txt -- +Add helper from maintainer +Edit feat1 locally +Add feat1 +Initial commit diff --git a/testdata/script/branch_submit_remote_diverged.txt b/testdata/script/branch_submit_remote_diverged.txt new file mode 100644 index 000000000..f0bab37ce --- /dev/null +++ b/testdata/script/branch_submit_remote_diverged.txt @@ -0,0 +1,67 @@ +# 'gs branch submit' refuses to push when the remote branch was rewritten +# away from our last push (its history no longer contains that commit), since +# it cannot tell what a force-push would discard. --force still overrides. + +as 'Test ' +at '2026-06-19T10:00:00Z' + +# setup +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +shamhub init +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +# create feat1 and submit it +git add feat1.txt +gs bc -m 'Add feat1' feat1 +gs branch submit --fill +stderr 'Created #' + +# Someone rewrites feat1 on the remote (force-push of a different history). +cd .. +git clone $SHAMHUB_URL/alice/example.git fork +cd fork +git checkout feat1 +cp $WORK/extra/feat1-rewrite.txt feat1.txt +git add feat1.txt +git commit --amend -m 'Rewritten feat1' +git push --force origin feat1 + +# Locally we make a change and try to submit. +cd ../repo +cp $WORK/extra/feat1-edit.txt feat1.txt +git add feat1.txt +git commit -m 'Edit feat1 locally' + +# Submit refuses: the last pushed commit is gone from the remote history. +! gs branch submit +stderr 'was rewritten and no longer contains your last push' +stderr 'use --force' + +# --force overrides and overwrites the remote. +gs branch submit --force +stderr 'Updated #' + +cd ../fork +git fetch +git log --pretty=%s origin/feat1 +cmp stdout $WORK/golden/remote-after-force.txt + +-- repo/feat1.txt -- +feat 1 +-- extra/feat1-edit.txt -- +feat 1 +edited locally +-- extra/feat1-rewrite.txt -- +totally different content +-- golden/remote-after-force.txt -- +Edit feat1 locally +Add feat1 +Initial commit