509893d fix(cli): require --head for mr merge · ermite

M cmd/ermite/cli_mr.go +9 -26


@@ -750,35 +750,31 @@ func newMRMergeCmd(env cliEnv) cobra.Command {
750	750		"as only parent, signed by the server's key (see `ermite signing-key show`). The target must be an\n" +
751	751		"ancestor of the head, else rebase the branch and push it again; the repository settings may allow\n" +
752	752		"only one of the modes.\n\n" +
753		-	"The server merges only if the source branch still points to the head you reviewed. --head gives\n" +
754		-	"that commit (the head of `mr show`, a full id). Without it the CLI reads the current head and merges\n" +
755		-	"that, which skips the safeguard: pass --head when others may push to the branch. Needs the\n" +
	753	+	"The server merges only if the source branch still points to the head you reviewed, so --head is\n" +
	754	+	"required: give the full id of the head that `ermite mr show <repo> <number>` printed when you\n" +
	755	+	"reviewed the change. If the branch moved since, the merge is refused: review again. Needs the\n" +
756	756		"mr:merge scope." + jsonMRHelp,
757	757		Args: cobra.ExactArgs(2),
758	758		ValidArgsFunction: completeMRRepoThenNumber(env),
759	759		}
760	760		cmd.Flags().BoolVar(&squash, "squash", false, "squash the branch into one signed commit instead of a fast-forward")
761		-	cmd.Flags().StringVar(&head, "head", "", "full id of the head you reviewed (default: the current head)")
	761	+	cmd.Flags().StringVar(&head, "head", "", "full id of the head you reviewed, from `ermite mr show` (required)")
762	762		asJSON := addJSONFlag(cmd)
763	763		cmd.RunE = func(cmd *cobra.Command, args []string) error {
764	764		n, err := parseNumber(args[1])
765	765		if err != nil {
766	766		return err
767	767		}
	768	+	head = strings.TrimSpace(head)
	769	+	if head == "" {
	770	+	return fmt.Errorf("--head is required: run `ermite mr show %s %d` and give the head id it prints, "+
	771	+	"so that only the commit you reviewed is merged", args[0], n)
	772	+	}
768	773		c, err := env.client(cmd, true)
769	774		if err != nil {
770	775		return err
771	776		}
772	777		ctx := env.ctx(cmd)
773		-	head = strings.TrimSpace(head)
774		-	if head == "" {
775		-	cur, err := currentHead(ctx, c, args[0], n)
776		-	if err != nil {
777		-	return err
778		-	}
779		-	head = cur
780		-	noticef(cmd, "merging %s!%d at %s (no --head given)", clean(args[0]), n, head)
781		-	}
782	778		mode := ermite.MergeMode_MERGE_MODE_FAST_FORWARD
783	779		if squash {
784	780		mode = ermite.MergeMode_MERGE_MODE_SQUASH

@@ -800,19 +796,6 @@ func newMRMergeCmd(env cliEnv) cobra.Command {
800	796		return cmd
801	797		}
802	798
803		-	// currentHead reads the head of a merge request from the server.
804		-	func currentHead(ctx context.Context, c *client.Client, repo string, n int64) (string, error) {
805		-	res, err := c.MergeRequests().GetMergeRequest(ctx, connect.NewRequest(&ermite.GetMergeRequestRequest{Repo: repo, Number: n}))
806		-	if err != nil {
807		-	return "", c.Describe(err)
808		-	}
809		-	head := res.Msg.GetMergeRequest().GetHeadId()
810		-	if head == "" {
811		-	return "", errors.New("the source branch no longer exists: nothing to merge")
812		-	}
813		-	return head, nil
814		-	}
815		-
816	799		func newMRCloseCmd(env cliEnv) cobra.Command {
817	800		cmd := &cobra.Command{
818	801		Use: "close <repo> <number>",

 			"as only parent, signed by the server's key (see `ermite signing-key show`). The target must be an\n" +
 			"ancestor of the head, else rebase the branch and push it again; the repository settings may allow\n" +
 			"only one of the modes.\n\n" +
-			"The server merges only if the source branch still points to the head you reviewed. --head gives\n" +
+			"The server merges only if the source branch still points to the head you reviewed, so --head is\n" +
-			"that commit (the head of `mr show`, a full id). Without it the CLI reads the current head and merges\n" +
+			"required: give the full id of the head that `ermite mr show <repo> <number>` printed when you\n" +
-			"that, which skips the safeguard: pass --head when others may push to the branch. Needs the\n" +
+			"reviewed the change. If the branch moved since, the merge is refused: review again. Needs the\n" +
 			"mr:merge scope." + jsonMRHelp,
 		Args:              cobra.ExactArgs(2),
 		ValidArgsFunction: completeMRRepoThenNumber(env),
 	}
 	cmd.Flags().BoolVar(&squash, "squash", false, "squash the branch into one signed commit instead of a fast-forward")
-	cmd.Flags().StringVar(&head, "head", "", "full id of the head you reviewed (default: the current head)")
+	cmd.Flags().StringVar(&head, "head", "", "full id of the head you reviewed, from `ermite mr show` (required)")
 	asJSON := addJSONFlag(cmd)
 	cmd.RunE = func(cmd *cobra.Command, args []string) error {
 		n, err := parseNumber(args[1])
 		if err != nil {
 			return err
 		}
+		head = strings.TrimSpace(head)
+		if head == "" {
+			return fmt.Errorf("--head is required: run `ermite mr show %s %d` and give the head id it prints, "+
+				"so that only the commit you reviewed is merged", args[0], n)
+		}
 		c, err := env.client(cmd, true)
 		if err != nil {
 			return err
 		}
 		ctx := env.ctx(cmd)
-		head = strings.TrimSpace(head)
-		if head == "" {
-			cur, err := currentHead(ctx, c, args[0], n)
-			if err != nil {
-				return err
-			}
-			head = cur
-			noticef(cmd, "merging %s!%d at %s (no --head given)", clean(args[0]), n, head)
-		}
 		mode := ermite.MergeMode_MERGE_MODE_FAST_FORWARD
 		if squash {
 			mode = ermite.MergeMode_MERGE_MODE_SQUASH
 	return cmd
 }
-// currentHead reads the head of a merge request from the server.
-func currentHead(ctx context.Context, c *client.Client, repo string, n int64) (string, error) {
-	res, err := c.MergeRequests().GetMergeRequest(ctx, connect.NewRequest(&ermite.GetMergeRequestRequest{Repo: repo, Number: n}))
-	if err != nil {
-		return "", c.Describe(err)
-	}
-	head := res.Msg.GetMergeRequest().GetHeadId()
-	if head == "" {
-		return "", errors.New("the source branch no longer exists: nothing to merge")
-	}
-	return head, nil
-}
 func newMRCloseCmd(env *cliEnv) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "close <repo> <number>",

M cmd/ermite/cli_mr_e2e_test.go +6 -4


@@ -259,16 +259,18 @@ func TestMergeRequestsCLIEndToEnd(t *testing.T) {
259	259		require.Error(t, res.err)
260	260		assert.Contains(t, res.err.Error(), "precondition failed")
261	261
262		-	// Squash, with --squash and without --head (the CLI reads the head and says so).
	262	+	// Squash, with --squash. Without --head the merge is refused before it reaches the server.
263	263		c.mustGit(wt, "checkout", "-q", "-b", "feat/two", head3)
264	264		c.mustGit(wt, "-c", "user.name=Cy", "-c", "user.email=cy@example.org", "commit", "-q", "--allow-empty", "-m", "empty by Cy")
265	265		c.writeFile(wt, "two.txt", []byte("two\n"))
266	266		c.mustGit(wt, "push", "-q", url, "feat/two")
267	267		c.cli(t, secret, "mr", "create", "proj", "--source", "feat/two", "--title", "Second").ok(t)
268	268		w.nextOfType(t, "merge_request_opened")
269		-	res = c.cli(t, secret, "mr", "merge", "proj", "2", "--squash")
	269	+	_, stderr, code = c.bin(t, secret, "mr", "merge", "proj", "2", "--squash")
	270	+	requireOneLineError(t, stderr, code, "--head is required", "ermite mr show proj 2")
	271	+	head4 := c.bareGit("proj", "rev-parse", "feat/two")
	272	+	res = c.cli(t, secret, "mr", "merge", "proj", "2", "--squash", "--head", head4)
270	273		require.NoError(t, res.err, res.stderr)
271		-	assert.Contains(t, res.stderr, "no --head given")
272	274		assert.Regexp(t, `^merged proj!2 into main $squash$ at [0-9a-f]{10}\n$`, res.stdout)
273	275		sq := c.bareGit("proj", "rev-parse", "main")
274	276		assert.Equal(t, head3, c.bareGit("proj", "rev-parse", sq+"^"))

@@ -335,7 +337,7 @@ func TestSigningKeyShowVerifiesASquashCommit(t *testing.T) {
335	337		c.writeFile(wt, "x.txt", []byte("x\n"))
336	338		c.mustGit(wt, "push", "-q", url, "main", "feat")
337	339		c.cli(t, adminToken, "mr", "create", "proj", "--source", "feat", "--title", "Add x").ok(t)
338		-	res := c.cli(t, adminToken, "mr", "merge", "proj", "1", "--squash")
	340	+	res := c.cli(t, adminToken, "mr", "merge", "proj", "1", "--squash", "--head", c.bareGit("proj", "rev-parse", "feat"))
339	341		require.NoError(t, res.err, res.stderr)
340	342		sq := c.bareGit("proj", "rev-parse", "main")
341	343

 	require.Error(t, res.err)
 	assert.Contains(t, res.err.Error(), "precondition failed")
-	// Squash, with --squash and without --head (the CLI reads the head and says so).
+	// Squash, with --squash. Without --head the merge is refused before it reaches the server.
 	c.mustGit(wt, "checkout", "-q", "-b", "feat/two", head3)
 	c.mustGit(wt, "-c", "user.name=Cy", "-c", "user.email=cy@example.org", "commit", "-q", "--allow-empty", "-m", "empty by Cy")
 	c.writeFile(wt, "two.txt", []byte("two\n"))
 	c.mustGit(wt, "push", "-q", url, "feat/two")
 	c.cli(t, secret, "mr", "create", "proj", "--source", "feat/two", "--title", "Second").ok(t)
 	w.nextOfType(t, "merge_request_opened")
-	res = c.cli(t, secret, "mr", "merge", "proj", "2", "--squash")
+	_, stderr, code = c.bin(t, secret, "mr", "merge", "proj", "2", "--squash")
+	requireOneLineError(t, stderr, code, "--head is required", "ermite mr show proj 2")
+	head4 := c.bareGit("proj", "rev-parse", "feat/two")
+	res = c.cli(t, secret, "mr", "merge", "proj", "2", "--squash", "--head", head4)
 	require.NoError(t, res.err, res.stderr)
-	assert.Contains(t, res.stderr, "no --head given")
 	assert.Regexp(t, `^merged proj!2 into main \(squash\) at [0-9a-f]{10}\n$`, res.stdout)
 	sq := c.bareGit("proj", "rev-parse", "main")
 	assert.Equal(t, head3, c.bareGit("proj", "rev-parse", sq+"^"))
 	c.writeFile(wt, "x.txt", []byte("x\n"))
 	c.mustGit(wt, "push", "-q", url, "main", "feat")
 	c.cli(t, adminToken, "mr", "create", "proj", "--source", "feat", "--title", "Add x").ok(t)
-	res := c.cli(t, adminToken, "mr", "merge", "proj", "1", "--squash")
+	res := c.cli(t, adminToken, "mr", "merge", "proj", "1", "--squash", "--head", c.bareGit("proj", "rev-parse", "feat"))
 	require.NoError(t, res.err, res.stderr)
 	sq := c.bareGit("proj", "rev-parse", "main")

M cmd/ermite/cli_mr_test.go +23 -14


@@ -271,12 +271,11 @@ func TestMRResolveMergeAndClose(t *testing.T) {
271	271		assert.Equal(t, headA, m.GetHeadId())
272	272		assert.EqualValues(t, 1, m.GetNumber())
273	273
274		-	// Without --head the CLI reads it, and says so.
275		-	res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--json").ok(t)
276		-	assert.Contains(t, res.stderr, "no --head given")
	274	+	// The default mode is a fast-forward.
	275	+	res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--json").ok(t)
277	276		assert.Equal(t, "merged", decode[mrOut](t, res.stdout).State)
278	277		calls, reqs = f.snapshot()
279		-	assert.Equal(t, []string{"GetMergeRequest", "MergeMergeRequest"}, calls[3:])
	278	+	assert.Equal(t, "MergeMergeRequest", calls[len(calls)-1])
280	279		m = reqs[len(reqs)-1].(*ermite.MergeMergeRequestRequest)
281	280		assert.Equal(t, ermite.MergeMode_MERGE_MODE_FAST_FORWARD, m.GetMode(), "fast-forward by default")
282	281		assert.Equal(t, headA, m.GetHeadId())

@@ -287,17 +286,27 @@ func TestMRResolveMergeAndClose(t *testing.T) {
287	286		assert.Equal(t, "closed", decode[mrOut](t, res.stdout).State)
288	287		}
289	288
290		-	func TestMRMergeWithoutHeadWhenTheBranchIsGone(t *testing.T) {
291		-	gone := proto.Clone(mrOne).(*ermite.MergeRequest)
292		-	gone.HeadId = ""
293		-	f := &fakeAPI{getMR: func(ermite.GetMergeRequestRequest) (ermite.GetMergeRequestResponse, error) {
294		-	return &ermite.GetMergeRequestResponse{MergeRequest: gone}, nil
295		-	}}
296		-	res := runCLI(t, envFor(f.serve(t)), "", "mr", "merge", "alpha", "1")
297		-	require.Error(t, res.err)
298		-	assert.Contains(t, res.err.Error(), "no longer exists")
	289	+	// ADR 0017: the merge is of the commit that was reviewed, so --head is
	290	+	// required, and the CLI does not fill it in from the live head.
	291	+	func TestMRMergeNeedsTheHead(t *testing.T) {
	292	+	f := &fakeAPI{}
	293	+	url := f.serve(t)
	294	+	for _, args := range [][]string{
	295	+	{"mr", "merge", "alpha", "1"},
	296	+	{"mr", "merge", "alpha", "1", "--squash"},
	297	+	{"mr", "merge", "alpha", "1", "--head", " "},
	298	+	} {
	299	+	res := runCLI(t, envFor(url), "", args...)
	300	+	require.Error(t, res.err, args)
	301	+	assert.Contains(t, res.err.Error(), "--head is required")
	302	+	assert.Contains(t, res.err.Error(), "ermite mr show alpha 1", "tells where to get the head")
	303	+	}
299	304		calls, _ := f.snapshot()
300		-	assert.Equal(t, []string{"GetMergeRequest"}, calls)
	305	+	assert.Empty(t, calls, "nothing reaches the server")
	306	+
	307	+	res := runCLI(t, envFor(url), "", "mr", "merge", "--help").ok(t)
	308	+	assert.Contains(t, res.stdout, "(required)")
	309	+	assert.NotContains(t, res.stdout, "default: the current head")
301	310		}
302	311
303	312		func TestMRErrorsAreFriendly(t *testing.T) {

 	assert.Equal(t, headA, m.GetHeadId())
 	assert.EqualValues(t, 1, m.GetNumber())
-	// Without --head the CLI reads it, and says so.
+	// The default mode is a fast-forward.
-	res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--json").ok(t)
+	res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--json").ok(t)
-	assert.Contains(t, res.stderr, "no --head given")
 	assert.Equal(t, "merged", decode[mrOut](t, res.stdout).State)
 	calls, reqs = f.snapshot()
-	assert.Equal(t, []string{"GetMergeRequest", "MergeMergeRequest"}, calls[3:])
+	assert.Equal(t, "MergeMergeRequest", calls[len(calls)-1])
 	m = reqs[len(reqs)-1].(*ermite.MergeMergeRequestRequest)
 	assert.Equal(t, ermite.MergeMode_MERGE_MODE_FAST_FORWARD, m.GetMode(), "fast-forward by default")
 	assert.Equal(t, headA, m.GetHeadId())
 	assert.Equal(t, "closed", decode[mrOut](t, res.stdout).State)
 }
-func TestMRMergeWithoutHeadWhenTheBranchIsGone(t *testing.T) {
+// ADR 0017: the merge is of the commit that was reviewed, so --head is
-	gone := proto.Clone(mrOne).(*ermite.MergeRequest)
+// required, and the CLI does not fill it in from the live head.
-	gone.HeadId = ""
+func TestMRMergeNeedsTheHead(t *testing.T) {
-	f := &fakeAPI{getMR: func(*ermite.GetMergeRequestRequest) (*ermite.GetMergeRequestResponse, error) {
+	f := &fakeAPI{}
-		return &ermite.GetMergeRequestResponse{MergeRequest: gone}, nil
+	url := f.serve(t)
-	}}
+	for _, args := range [][]string{
-	res := runCLI(t, envFor(f.serve(t)), "", "mr", "merge", "alpha", "1")
+		{"mr", "merge", "alpha", "1"},
-	require.Error(t, res.err)
+		{"mr", "merge", "alpha", "1", "--squash"},
-	assert.Contains(t, res.err.Error(), "no longer exists")
+		{"mr", "merge", "alpha", "1", "--head", "  "},
+	} {
+		res := runCLI(t, envFor(url), "", args...)
+		require.Error(t, res.err, args)
+		assert.Contains(t, res.err.Error(), "--head is required")
+		assert.Contains(t, res.err.Error(), "ermite mr show alpha 1", "tells where to get the head")
+	}
 	calls, _ := f.snapshot()
-	assert.Equal(t, []string{"GetMergeRequest"}, calls)
+	assert.Empty(t, calls, "nothing reaches the server")
+	res := runCLI(t, envFor(url), "", "mr", "merge", "--help").ok(t)
+	assert.Contains(t, res.stdout, "(required)")
+	assert.NotContains(t, res.stdout, "default: the current head")
 }
 func TestMRErrorsAreFriendly(t *testing.T) {