cmd/cueckoo: drop --change flag

When being given arguments, use a regular expression to see
if an argument looks like a CL number or full Change-Id string.

It is possible for one to name a git branch to match those forms,
but I don't think that is a likely scenario.
Moreover, the most common git ref use case is either a git commit hash
or HEAD to test all commits from the branchpoint.

This bites me every time I want to run the trybots on someone else's CL,
since I give their CL number or Change-Id to avoid having to fetch it.

While here, noticed that resolveCommits could panic if git log
returned an empty list, since we would Split the resulting empty string
and try to index into the missing space-separated fields, panicking.

I also noticed that we still asked the user to set up GITHUB_USER
and GITHUB_PAT, which hasn't been a strict requirement
since we added support for git credential helpers.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: I529dfa5401625f1546971ed9520d0fe4f35d6a10
Reviewed-on: https://review.gerrithub.io/c/cue-sh/tools/+/1171039
Reviewed-by: Paul Jolly <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>

cmd/cueckoo/cmd/cltrigger.go

@@ -45,36 +45,58 @@ func newCLTrigger(cmd *Command, cfg *config, b builder) *cltrigger {
 	}
 }
 
+// Per https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id,
+// a change ID can be in multiple forms.
+// We only really care about CL numbers and Change-ID identifiers from git commit trailers,
+// since those are what a human user is most likely going to find useful.
+// The long forms, like "project~branch~I1234..." are far too cumbersome.
+var rxChangeID = regexp.MustCompile(`^[1-9][0-9]{3,}|I[0-9a-f]{40}$`)
+
 func (c *cltrigger) run() (err error) {
 	var changeIDs []revision
-	args := make(map[string]bool)
-	for _, a := range c.cmd.Flags().Args() {
-		args[a] = true
+	args := c.cmd.Flags().Args()
+	derive := true
+	for _, arg := range args {
+		if rxChangeID.MatchString(arg) {
+			derive = false
+		} else if !derive {
+			return fmt.Errorf("cannot mix change IDs and git refs")
+		}
 	}
-	if flagChange.Bool(c.cmd) {
+	if derive {
+		changeIDs, err = c.deriveChangeIDs(args)
+		if err != nil {
+			return err
+		}
+	} else {
 		if len(args) == 0 {
 			return fmt.Errorf("must provide at least one change number of ID")
 		}
-		for a := range args {
+		for _, a := range args {
 			changeIDs = append(changeIDs, revision{
 				changeID: a,
 			})
 		}
-	} else {
-		changeIDs, err = c.deriveChangeIDs(args)
-		if err != nil {
-			return err
-		}
 	}
 	return c.triggerBuilds(changeIDs)
 }
 
+// TODO: replace once we can use slices.Contains
+func slicesContains[S ~[]E, E comparable](s S, v E) bool {
+	for i := range s {
+		if v == s[i] {
+			return true
+		}
+	}
+	return false
+}
+
 // deriveChangeIDs determines a list of change IDs for the supplied args (if
 // there are any). See the trybot docs for an explanation of what is derived.
 // Essentially however we try to follow the semantics of git-codereview:
 //
 // https://pkg.go.dev/golang.org/x/review/git-codereview
-func (c *cltrigger) deriveChangeIDs(args map[string]bool) (res []revision, err error) {
+func (c *cltrigger) deriveChangeIDs(args []string) (res []revision, err error) {
 	ctx := context.TODO()
 	// Work out the branchpoint
 	bp, err := run(ctx, "git", "codereview", "branchpoint")
@@ -91,10 +113,11 @@ func (c *cltrigger) deriveChangeIDs(args map[string]bool) (res []revision, err e
 	if len(pendingCommits) == 0 {
 		return nil, fmt.Errorf("no pending commits")
 	}
-	if args["HEAD"] && len(args) > 1 {
+	argHead := slicesContains(args, "HEAD")
+	if argHead && len(args) > 1 {
 		return nil, fmt.Errorf("HEAD can only be supplied as an argument by itself")
 	}
-	if !args["HEAD"] && len(pendingCommits) > 1 && len(args) == 0 {
+	if !argHead && len(pendingCommits) > 1 && len(args) == 0 {
 		return nil, fmt.Errorf("must specify commits as arguments or use HEAD for everything")
 	}
 	addRevision := func(pc commit) error {
@@ -124,7 +147,7 @@ func (c *cltrigger) deriveChangeIDs(args map[string]bool) (res []revision, err e
 		})
 		return nil
 	}
-	if args["HEAD"] || len(args) == 0 && len(pendingCommits) == 1 {
+	if argHead || len(args) == 0 && len(pendingCommits) == 1 {
 		// Verify all
 		for _, pc := range pendingCommits {
 			if err := addRevision(pc); err != nil {
@@ -137,7 +160,7 @@ func (c *cltrigger) deriveChangeIDs(args map[string]bool) (res []revision, err e
 
 		// We verify each of the arguments
 	EachArg:
-		for h := range args {
+		for _, h := range args {
 			// Resolve the arg and ensure we have a matching pending commit
 			// and ensure we have a single one
 			commits, err := resolveCommits(ctx, "-1", h)
@@ -177,6 +200,9 @@ func resolveCommits(ctx context.Context, args ...string) ([]commit, error) {
 	if err != nil {
 		return nil, err
 	}
+	if commitStream == "" {
+		return nil, fmt.Errorf("%q resulted in no git commits", args)
+	}
 
 	var commits []commit
 

cmd/cueckoo/cmd/runtrybot.go

@@ -22,7 +22,6 @@ import (
 )
 
 const (
-	flagChange           flagName = "change"
 	flagRunTrybotNoUnity flagName = "nounity"
 	flagForce            flagName = "force"
 )
@@ -35,31 +34,25 @@ func newRuntrybotCmd(c *Command) *cobra.Command {
 		Long: `
 Usage of runtrybot:
 
-	runtrybot [--change] [--nounity] [ARGS...]
+	runtrybot [--nounity] [ARGS...]
 
 Triggers trybot and unity runs for its arguments.
 
 When run with no arguments, runtrybot derives a revision and change ID for each
 pending commit in the current branch. If multiple pending commits are found,
-you must either specify which commits to run, or specify HEAD to run the
+you must either specify which commits or CLs to run, or specify HEAD to run the
 trybots for all of them.
 
-If the --change flag is provide, then the list of arguments is interpreted as
-change numbers or IDs, and the latest revision from each of those changes is
-assumed.
-
-runtrybot requires GITHUB_USER and GITHUB_PAT environment variables to be set
-with your GitHub username and personal acccess token respectively. The personal
-access token requires the "repo" scope, since Unity is a private repository.
-
+runtrybot needs your GitHub username and a personal acccess token
+with the "repo" scope. You can configure them via your git credential helper,
+or by setting the GITHUB_USER and GITHUB_PAT environment variables.
 Note that the personal access token should be "classic"; GitHub's new
 fine-grained tokens are still in beta and haven't been tested to work here.
 
 If the --nounity flag is provided, only a trybot run is triggered.
 `,
 		RunE: mkRunE(c, runtrybotDef),
 	}
-	cmd.Flags().Bool(string(flagChange), false, "interpret arguments as change numbers or IDs")
 	cmd.Flags().Bool(string(flagRunTrybotNoUnity), false, "do not simultaenously trigger unity build")
 	cmd.Flags().BoolP(string(flagForce), string(flagForce[0]), false, "force the trybots to run, ignoring any results")
 	return cmd

cmd/cueckoo/cmd/unity.go

@@ -45,13 +45,14 @@ unity for all of them.
 If the --normal flag is provided, then the list of arguments is interpreted as
 versions understood by unity.
 
-unity requires GITHUB_USER and GITHUB_PAT environment variables to be set with
-your GitHub username and personal acccess token respectively. The personal
-access token requires the "repo" scope, since Unity is a private repository.
+runtrybot needs your GitHub username and a personal acccess token
+with the "repo" scope. You can configure them via your git credential helper,
+or by setting the GITHUB_USER and GITHUB_PAT environment variables.
+Note that the personal access token should be "classic"; GitHub's new
+fine-grained tokens are still in beta and haven't been tested to work here.
 `,
 		RunE: mkRunE(c, unityDef),
 	}
-	cmd.Flags().Bool(string(flagChange), false, "interpret arguments as change numbers or IDs")
 	cmd.Flags().Bool(string(flagUnityVersions), false, "pass arguments to unity as versions")
 	return cmd
 }
@@ -62,10 +63,6 @@ func unityDef(cmd *Command, args []string) error {
 		return err
 	}
 
-	if flagUnityVersions.Bool(cmd) && flagChange.Bool(cmd) {
-		return fmt.Errorf("cannot supply --change and --versions")
-	}
-
 	// If we are passed --normal, interpret all args as versions to be passed to
 	// unity
 	if flagUnityVersions.Bool(cmd) {