submit: --no-publish should work for unsupported forges

If using `submit` commands with --no-publish,
there's no need to require remote repository information.
As long as we can `git push` to it, we're good.

This commit reorganizes the information retrieval in submit
commands to only fetch the remote repository information
when necessary.

Resolves #351

.changes/unreleased/Fixed-20240821-082941.yaml

@@ -0,0 +1,3 @@
+kind: Fixed
+body: '{branch, stack, upstack, downstack} submit: When submitting with --no-publish, allow pushing to repositories managed by unsupported forges.'
+time: 2024-08-21T08:29:41.537238-07:00

branch_submit.go

@@ -99,19 +99,13 @@ func (cmd *branchSubmitCmd) Run(
 		return nil
 	}
 
-	remoteRepo, err := session.RemoteRepo.Get(ctx)
-	if err != nil {
-		return err
-	}
-
 	return syncStackComments(
 		ctx,
 		store,
 		svc,
-		remoteRepo,
 		log,
 		cmd.NavigationComment,
-		session.branches,
+		session,
 	)
 }
 
@@ -175,16 +169,16 @@ func (cmd *branchSubmitCmd) run(
 		return err
 	}
 
-	remoteRepo, err := session.RemoteRepo.Get(ctx)
-	if err != nil {
-		return err
-	}
-
-	// If the branch doesn't have a CR associated with it,
-	// we'll probably need to create one,
-	// but verify that there isn't already one open.
 	var existingChange *forge.FindChangeItem
-	if branch.Change == nil {
+	if branch.Change == nil && !cmd.NoPublish {
+		// If the branch doesn't have a CR associated with it,
+		// we'll probably need to create one,
+		// but verify that there isn't already one open.
+		remoteRepo, err := session.RemoteRepo.Get(ctx)
+		if err != nil {
+			return fmt.Errorf("discover CR for %s: %w", cmd.Branch, err)
+		}
+
 		changes, err := remoteRepo.FindChangesByBranch(ctx, upstreamBranch, forge.FindChangesOptions{
 			State: forge.ChangeOpen,
 			Limit: 3,
@@ -237,13 +231,19 @@ func (cmd *branchSubmitCmd) run(
 			return fmt.Errorf("multiple open change requests for %s", cmd.Branch)
 			// TODO: Ask the user to pick one and associate it with the branch.
 		}
-	} else {
+	} else if branch.Change != nil {
+		remoteRepo, err := session.RemoteRepo.Get(ctx)
+		if err != nil {
+			return fmt.Errorf("look up CR %v: %w", branch.Change.ChangeID(), err)
+		}
+
 		// If a CR is already associated with the branch,
 		// fetch information about it to compare with the current state.
 		change, err := remoteRepo.FindChangeByID(ctx, branch.Change.ChangeID())
 		if err != nil {
 			return fmt.Errorf("find change: %w", err)
 		}
+
 		// TODO: If the CR is closed, we should treat it as non-existent.
 		existingChange = change
 	}
@@ -261,6 +261,15 @@ func (cmd *branchSubmitCmd) run(
 
 		var prepared *preparedBranch
 		if !cmd.NoPublish {
+			remoteRepo, err := session.RemoteRepo.Get(ctx)
+			if err != nil {
+				return fmt.Errorf("prepare publish: %w", err)
+			}
+
+			// TODO: Refactor:
+			// NoPublish and DryRun are checked repeatedly.
+			// Extract the logic that needs them into no-ops
+			// and make this function flow more linearly.
 			prepared, err = cmd.preparePublish(
 				ctx,
 				log,
@@ -328,6 +337,7 @@ func (cmd *branchSubmitCmd) run(
 				return err
 			}
 
+			remoteRepo := prepared.remoteRepo
 			changeMeta, err := remoteRepo.NewChangeMetadata(ctx, changeID)
 			if err != nil {
 				return fmt.Errorf("get change metadata: %w", err)
@@ -403,6 +413,12 @@ func (cmd *branchSubmitCmd) run(
 				Draft: cmd.Draft,
 			}
 
+			// remoteRepo is guaranteed to be available at this point.
+			remoteRepo, err := session.RemoteRepo.Get(ctx)
+			if err != nil {
+				return fmt.Errorf("edit CR %v: %w", pull.ID, err)
+			}
+
 			if err := remoteRepo.EditChange(ctx, pull.ID, opts); err != nil {
 				return fmt.Errorf("edit CR %v: %w", pull.ID, err)
 			}

downstack_submit.go

@@ -74,18 +74,12 @@ func (cmd *downstackSubmitCmd) Run(
 		return nil
 	}
 
-	remoteRepo, err := session.RemoteRepo.Get(ctx)
-	if err != nil {
-		return err
-	}
-
 	return syncStackComments(
 		ctx,
 		store,
 		svc,
-		remoteRepo,
 		log,
 		cmd.NavigationComment,
-		session.branches,
+		session,
 	)
 }

internal/forge/shamhub/forge.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/charmbracelet/log"
 	"go.abhg.dev/gs/internal/forge"
-	"go.abhg.dev/gs/internal/must"
 )
 
 // Options defines CLI options for the ShamHub forge.
@@ -51,7 +50,10 @@ func (f *Forge) CLIPlugin() any { return &f.Options }
 
 // MatchURL reports whether the given URL is a ShamHub URL.
 func (f *Forge) MatchURL(remoteURL string) bool {
-	must.NotBeBlankf(f.URL, "URL is required")
+	if f.URL == "" {
+		// ShamHub is not initialized.
+		return false
+	}
 
 	_, ok := strings.CutPrefix(remoteURL, f.URL)
 	return ok

stack_submit.go

@@ -63,18 +63,12 @@ func (cmd *stackSubmitCmd) Run(
 		return nil
 	}
 
-	remoteRepo, err := session.RemoteRepo.Get(ctx)
-	if err != nil {
-		return err
-	}
-
 	return syncStackComments(
 		ctx,
 		store,
 		svc,
-		remoteRepo,
 		log,
 		cmd.NavigationComment,
-		session.branches,
+		session,
 	)
 }

submit.go

@@ -136,11 +136,20 @@ func syncStackComments(
 	ctx context.Context,
 	store *state.Store,
 	svc *spice.Service,
-	remoteRepo forge.Repository,
 	log *log.Logger,
 	navComment navigationCommentWhen,
-	submittedBranches []string,
+	session *submitSession,
 ) error {
+	submittedBranches := session.branches
+	if len(submittedBranches) == 0 {
+		return nil
+	}
+
+	remoteRepo, err := session.RemoteRepo.Get(ctx)
+	if err != nil {
+		return fmt.Errorf("resolve remote repository: %w", err)
+	}
+
 	if navComment == navigationCommentNever {
 		return nil // nothing to do
 	}

testdata/script/issue351_submit_no_publish_unsupported_forge.txt

@@ -0,0 +1,85 @@
+# submit commands should still be able to push to unsupported forges
+# if the --no-publish flag is used.
+#
+# https://github.com/abhinav/git-spice/issues/351
+
+as 'Test <[email protected]>'
+at '2024-08-21T05:18:00Z'
+
+# setup an upstream repository
+mkdir upstream
+cd upstream
+git init
+git commit --allow-empty -m 'Initial commit'
+
+# receive updates to the current branch
+git config receive.denyCurrentBranch updateInstead
+
+# setup the git-spice managed repository
+cd ..
+git clone upstream repo
+cd repo
+gs repo init
+
+# set up a stack: feat1 -> feat2 -> feat3
+mv $WORK/extra/feat1.txt feat1.txt
+git add feat1.txt
+gs bc -m feat1
+
+mv $WORK/extra/feat2.txt feat2.txt
+git add feat2.txt
+gs bc -m feat2
+
+mv $WORK/extra/feat3.txt feat3.txt
+git add feat3.txt
+gs bc -m feat3
+
+# branch submit: supports pushing
+gs bottom
+gs branch submit --no-publish
+
+# downstack submit: supports pushing
+gs up
+gs downstack submit --no-publish
+
+# upstack submit: supports pushing
+gs upstack submit --no-publish
+
+# stack submit: supports pushing
+gs stack submit --no-publish
+
+# submit: can push a new commit
+gs bottom
+mv $WORK/extra/feat1-new.txt feat1.txt
+git add feat1.txt
+gs cc -m 'feat1 new version'
+gs stack submit --no-publish
+
+# submit: can push an amended commit
+gs up
+mv $WORK/extra/feat2-new.txt feat2.txt
+git add feat2.txt
+gs ca -m 'feat2 new version'
+gs stack submit --no-publish
+
+# verify final state
+cd ../upstream
+git graph --branches
+cmp stdout $WORK/golden/final-graph.txt
+
+-- extra/feat1.txt --
+feature 1
+-- extra/feat2.txt --
+feature 2
+-- extra/feat3.txt --
+feature 3
+-- extra/feat1-new.txt --
+feature 1 new version
+-- extra/feat2-new.txt --
+feature 2 new version
+-- golden/final-graph.txt --
+* 34273ae (feat3) feat3
+* b7b3597 (feat2) feat2 new version
+* a5db510 (feat1) feat1 new version
+* 85d6296 feat1
+* 019537e (HEAD -> main) Initial commit

upstack_submit.go

@@ -57,7 +57,7 @@ func (cmd *upstackSubmitCmd) Run(
 				return fmt.Errorf("lookup base %v: %w", b.Base, err)
 			}
 
-			if base.Change == nil {
+			if base.Change == nil && !cmd.NoPublish {
 				log.Errorf("%v: base (%v) has not been submitted", cmd.Branch, b.Base)
 				return errors.New("submit the base branch first")
 			}
@@ -92,18 +92,12 @@ func (cmd *upstackSubmitCmd) Run(
 		return nil
 	}
 
-	remoteRepo, err := session.RemoteRepo.Get(ctx)
-	if err != nil {
-		return err
-	}
-
 	return syncStackComments(
 		ctx,
 		store,
 		svc,
-		remoteRepo,
 		log,
 		cmd.NavigationComment,
-		session.branches,
+		session,
 	)
 }