ronantakizawa/codereview-bench huggingface dataset:
Repository: cli/cli
File: pkg/cmd/browse/browse.go (Go)

Code before:
	WikiFlag        bool
	NoBrowserFlag   bool
	HasRepoOverride bool
}

func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Command {
	opts := &BrowseOptions{
		Browser:    f.Browser,
		HttpClient: f.HttpClient,
		IO:         f.IOStreams,
		PathFromRepoRoot: func() string {
			return f.GitClient.PathFromRoot(context.Background())
		},
		GitClient: &localGitClient{client: f.GitClient},
	}

	cmd := &cobra.Command{
		Short: "Open repositories, issues, pull requests, and more in the browser",
		Long:  heredoc.Doc(`
			Transition from the terminal to the web browser to view and interact with:

			- issues
			- pull requests
			- repository content
			- repository profile
			- repository settings
		`),
		Use:   "browse [<number> | <path> | <commit-SHA>]",
		Args:  cobra.MaximumNArgs(1),
		Example: heredoc.Doc(`
			$ gh browse
			#=> Open the home page of the current repository

			$ gh browse script/
			#=> Open the script directory of the current repository

			$ gh browse 217
			#=> Open issue or pull request 217

			$ gh browse 77507cd94ccafcf568f8560cfecde965fcfa63
			#=> Open commit page

			$ gh browse --settings
			#=> Open repository settings

			$ gh browse main.go:312
			#=> Open main.go at line 312

			$ gh browse main.go --branch bug-fix
			#=> Open main.go with the repository at head of bug-fix branch


Review comment (suggestion):
I think we should sentence case these 🙂 

```suggestion
			Transition from the terminal to the web browser to view and interact with:

			- Issues
			- Pull requests
			- Repository content
			- Repository profile
			- Repository settings
```

Code after:
	WikiFlag        bool
	NoBrowserFlag   bool
	HasRepoOverride bool
}

func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Command {
	opts := &BrowseOptions{
		Browser:    f.Browser,
		HttpClient: f.HttpClient,
		IO:         f.IOStreams,
		PathFromRepoRoot: func() string {
			return f.GitClient.PathFromRoot(context.Background())
		},
		GitClient: &localGitClient{client: f.GitClient},
	}

	cmd := &cobra.Command{
		Short: "Open repositories, issues, pull requests, and more in the browser",
		Long: heredoc.Doc(`
			Transition from the terminal to the web browser to view and interact with:

			- Issues
			- Pull requests
			- Repository content
			- Repository home page
			- Repository settings
		`),
		Use:  "browse [<number> | <path> | <commit-SHA>]",
		Args: cobra.MaximumNArgs(1),
		Example: heredoc.Doc(`
			$ gh browse
			#=> Open the home page of the current repository

			$ gh browse script/
			#=> Open the script directory of the current repository

			$ gh browse 217
			#=> Open issue or pull request 217

			$ gh browse 77507cd94ccafcf568f8560cfecde965fcfa63
			#=> Open commit page

			$ gh browse --settings
			#=> Open repository settings

			$ gh browse main.go:312
			#=> Open main.go at line 312

			$ gh browse main.go --branch bug-fix
			#=> Open main.go with the repository at head of bug-fix branch
Repository: cli/cli
File: pkg/cmd/browse/browse.go (Go)

Code before:
	SettingsFlag    bool
	WikiFlag        bool
	NoBrowserFlag   bool
	HasRepoOverride bool
}

func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Command {
	opts := &BrowseOptions{
		Browser:    f.Browser,
		HttpClient: f.HttpClient,
		IO:         f.IOStreams,
		PathFromRepoRoot: func() string {
			return f.GitClient.PathFromRoot(context.Background())
		},
		GitClient: &localGitClient{client: f.GitClient},
	}

	cmd := &cobra.Command{
		Short: "Open repositories, issues, pull requests, and more in the browser",
		Long: heredoc.Doc(`
			Transition from the terminal to the web browser to view and interact with:

			- Issues
			- Pull requests
			- Repository content
			- Repository profile
			- Repository settings
		`),
		Use:  "browse [<number> | <path> | <commit-SHA>]",
		Args: cobra.MaximumNArgs(1),
		Example: heredoc.Doc(`
			$ gh browse
			#=> Open the home page of the current repository

			$ gh browse script/
			#=> Open the script directory of the current repository

			$ gh browse 217
			#=> Open issue or pull request 217

			$ gh browse 77507cd94ccafcf568f8560cfecde965fcfa63
			#=> Open commit page

			$ gh browse --settings
			#=> Open repository settings

			$ gh browse main.go:312
			#=> Open main.go at line 312

			$ gh browse main.go --branch bug-fix
			#=> Open main.go with the repository at head of bug-fix branch

Review comment (suggestion):
Apologies, one more suggestion. I think this should be changed to line up with the wording of the examples 😁 

```suggestion
			- Repository home page
```

Code after:
	SettingsFlag    bool
	WikiFlag        bool
	NoBrowserFlag   bool
	HasRepoOverride bool
}

func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Command {
	opts := &BrowseOptions{
		Browser:    f.Browser,
		HttpClient: f.HttpClient,
		IO:         f.IOStreams,
		PathFromRepoRoot: func() string {
			return f.GitClient.PathFromRoot(context.Background())
		},
		GitClient: &localGitClient{client: f.GitClient},
	}

	cmd := &cobra.Command{
		Short: "Open repositories, issues, pull requests, and more in the browser",
		Long: heredoc.Doc(`
			Transition from the terminal to the web browser to view and interact with:

			- Issues
			- Pull requests
			- Repository content
			- Repository home page
			- Repository settings
		`),
		Use:  "browse [<number> | <path> | <commit-SHA>]",
		Args: cobra.MaximumNArgs(1),
		Example: heredoc.Doc(`
			$ gh browse
			#=> Open the home page of the current repository

			$ gh browse script/
			#=> Open the script directory of the current repository

			$ gh browse 217
			#=> Open issue or pull request 217

			$ gh browse 77507cd94ccafcf568f8560cfecde965fcfa63
			#=> Open commit page

			$ gh browse --settings
			#=> Open repository settings

			$ gh browse main.go:312
			#=> Open main.go at line 312

			$ gh browse main.go --branch bug-fix
			#=> Open main.go with the repository at head of bug-fix branch
Repository: cli/cli
File: pkg/cmd/gist/edit/edit.go (Go)

Code before:
	cmd.MarkFlagsMutuallyExclusive("add", "remove")
	cmd.MarkFlagsMutuallyExclusive("remove", "filename")

	return cmd
}

func editRun(opts *EditOptions) error {
	client, err := opts.HttpClient()
	if err != nil {
		return err
	}

	cfg, err := opts.Config()
	if err != nil {
		return err
	}

	host, _ := cfg.Authentication().DefaultHost()

	gistID := opts.Selector
	if gistID == "" {
		cs := opts.IO.ColorScheme()
		if gistID == "" {
			if !opts.IO.CanPrompt() {
				return cmdutil.FlagErrorf("gist ID or URL required when not running interactively")
			}
			gistID, err = shared.PromptGists(opts.Prompter, client, host, cs)
			if err != nil {
				return err
			}

			if gistID == "" {
				fmt.Fprintln(opts.IO.Out, "No gists found.")
				return nil
			}
		}
	}

	if strings.Contains(gistID, "/") {
		id, err := shared.GistIDFromURL(gistID)
		if err != nil {
			return err
		}
		gistID = id
	}

	apiClient := api.NewClientFromHTTP(client)

	gist, err := shared.GetGist(client, host, gistID)
	if err != nil {
		if errors.Is(err, shared.NotFoundErr) {

Review comment (suggestion):
Nit: I think we should keep the spacing consistent between the changes to `gist edit` and `gist view` 😄  

```suggestion
			if !opts.IO.CanPrompt() {
				return cmdutil.FlagErrorf("gist ID or URL required when not running interactively")
			}
			
```

Code after:
	cmd.MarkFlagsMutuallyExclusive("add", "remove")
	cmd.MarkFlagsMutuallyExclusive("remove", "filename")

	return cmd
}

func editRun(opts *EditOptions) error {
	client, err := opts.HttpClient()
	if err != nil {
		return err
	}

	cfg, err := opts.Config()
	if err != nil {
		return err
	}

	host, _ := cfg.Authentication().DefaultHost()

	gistID := opts.Selector
	if gistID == "" {
		cs := opts.IO.ColorScheme()
		if gistID == "" {
			if !opts.IO.CanPrompt() {
				return cmdutil.FlagErrorf("gist ID or URL required when not running interactively")
			}

			gist, err := shared.PromptGists(opts.Prompter, client, host, cs)
			if err != nil {
				return err
			}

			if gist.ID == "" {
				fmt.Fprintln(opts.IO.Out, "No gists found.")
				return nil
			}
			gistID = gist.ID
		}
	}

	if strings.Contains(gistID, "/") {
		id, err := shared.GistIDFromURL(gistID)
		if err != nil {
			return err
		}
		gistID = id
	}

	apiClient := api.NewClientFromHTTP(client)

	gist, err := shared.GetGist(client, host, gistID)
Repository: cli/cli
File: pkg/cmd/gist/delete/delete.go (Go)

Code before:
		if errors.Is(err, shared.NotFoundErr) {
			return fmt.Errorf("unable to delete gist %q: either the gist is not found or it is not owned by you", gist.FilenameDescription())
		}
		return err
	}

	return nil
}

func deleteGist(apiClient *api.Client, hostname string, gist *shared.Gist, opts *DeleteOptions) error {
	path := "gists/" + gist.ID
	err := apiClient.REST(hostname, "DELETE", path, nil, nil)
	if err != nil {
		var httpErr api.HTTPError
		if errors.As(err, &httpErr) && httpErr.StatusCode == 404 {
			return shared.NotFoundErr
		}
		return err
	}
	if opts.IO.IsStdoutTTY() {
		cs := opts.IO.ColorScheme()
		fmt.Fprintf(opts.IO.Out,
			"%s Gist %q deleted\n",
			cs.SuccessIcon(),
			gist.FilenameDescription())
	}
	return nil
}


Review comment (suggestion):
I think I'd prefer this to be in `deleteRun` instead of in this `deleteGist` function. I think that what I expect this function to do is hit the API and return success or error. Let's let the command decide what to do with that

Code after:
	}

	apiClient := api.NewClientFromHTTP(client)
	if err := deleteGist(apiClient, host, gist.ID); err != nil {
		if errors.Is(err, shared.NotFoundErr) {
			return fmt.Errorf("unable to delete gist %q: either the gist is not found or it is not owned by you", gist.Filename())
		}
		return err
	}

	if opts.IO.IsStdoutTTY() {
		cs := opts.IO.ColorScheme()
		fmt.Fprintf(opts.IO.Out,
			"%s Gist %q deleted\n",
			cs.SuccessIcon(),
			gist.Filename())
	}

	return nil
}

func deleteGist(apiClient *api.Client, hostname string, gistID string) error {
	path := "gists/" + gistID
	err := apiClient.REST(hostname, "DELETE", path, nil, nil)
	if err != nil {
		var httpErr api.HTTPError
		if errors.As(err, &httpErr) && httpErr.StatusCode == 404 {
			return shared.NotFoundErr
		}
		return err
	}
	return nil
}
Repository: cli/cli
File: pkg/cmd/attestation/api/client.go (Go)

Code before:
			}

			url = newURL
			attestations = append(attestations, resp.Attestations...)

			return nil
		}, backoff.WithMaxRetries(bo, 3))

		// bail if RESTWithNext errored out
		if err != nil {
			return nil, err
		}
	}

	if len(attestations) == 0 {
		return nil, newErrNoAttestations(name, digest)
	}

	if len(attestations) > limit {
		return attestations[:limit], nil
	}

	return attestations, nil
}

func (c *LiveClient) fetchBundlesByURL(attestations []*Attestation) ([]*Attestation, error) {
	fetched := make([]*Attestation, len(attestations))
	g := errgroup.Group{}
	for i, a := range attestations {
		g.Go(func() error {
			b, err := c.fetchBundleByURL(a)
			if err != nil {
				return fmt.Errorf("failed to fetch bundle with URL: %w", err)
			}
			fetched[i] = &Attestation{
				Bundle: b,
			}
			return nil
		})
	}

	if err := g.Wait(); err != nil {
		return nil, err
	}

	return fetched, nil
}

func (c *LiveClient) fetchBundleByURL(a *Attestation) (*bundle.Bundle, error) {
	// for now, we fallback to the bundle field if the bundle URL is empty
	if a.BundleURL == "" {

Review comment (suggestion):
minor nitpick re: naming,

if the func arg is an _attestation_ what if?

```suggestion
func (c *LiveClient) fetchAttestationBundle(attestations []*Attestation) ([]*Attestation, error) {
```

Code after:
			}

			url = newURL
			attestations = append(attestations, resp.Attestations...)

			return nil
		}, backoff.WithMaxRetries(bo, 3))

		// bail if RESTWithNext errored out
		if err != nil {
			return nil, err
		}
	}

	if len(attestations) == 0 {
		return nil, newErrNoAttestations(name, digest)
	}

	if len(attestations) > limit {
		return attestations[:limit], nil
	}

	return attestations, nil
}

func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([]*Attestation, error) {
	fetched := make([]*Attestation, len(attestations))
	g := errgroup.Group{}
	for i, a := range attestations {
		g.Go(func() error {
			if a.Bundle == nil && a.BundleURL == "" {
				return fmt.Errorf("attestation has no bundle or bundle URL")
			}

			// for now, we fallback to the bundle field if the bundle URL is empty
			if a.BundleURL == "" {
				c.logger.VerbosePrintf("Bundle URL is empty. Falling back to bundle field\n\n")
				fetched[i] = &Attestation{
					Bundle: a.Bundle,
				}
				return nil
			}

			// otherwise fetch the bundle with the provided URL
			b, err := c.GetBundle(a.BundleURL)
			if err != nil {
				return fmt.Errorf("failed to fetch bundle with URL: %w", err)
			}
			fetched[i] = &Attestation{
				Bundle: b,
			}
Repository: cli/cli
File: pkg/cmd/attestation/api/client.go (Go)

Code before:
			return nil
		}, backoff.WithMaxRetries(bo, 3))

		// bail if RESTWithNext errored out
		if err != nil {
			return nil, err
		}
	}

	if len(attestations) == 0 {
		return nil, newErrNoAttestations(name, digest)
	}

	if len(attestations) > limit {
		return attestations[:limit], nil
	}

	return attestations, nil
}

func (c *LiveClient) fetchBundlesByURL(attestations []*Attestation) ([]*Attestation, error) {
	fetched := make([]*Attestation, len(attestations))
	g := errgroup.Group{}
	for i, a := range attestations {
		g.Go(func() error {
			b, err := c.fetchBundleByURL(a)
			if err != nil {
				return fmt.Errorf("failed to fetch bundle with URL: %w", err)
			}
			fetched[i] = &Attestation{
				Bundle: b,
			}
			return nil
		})
	}

	if err := g.Wait(); err != nil {
		return nil, err
	}

	return fetched, nil
}

func (c *LiveClient) fetchBundleByURL(a *Attestation) (*bundle.Bundle, error) {
	// for now, we fallback to the bundle field if the bundle URL is empty
	if a.BundleURL == "" {
		c.logger.VerbosePrintf("Bundle URL is empty. Falling back to bundle field\n\n")
		return a.Bundle, nil
	}

	c.logger.VerbosePrintf("Fetching attestation bundle with bundle URL\n\n")

Review comment (suggestion):
and then here, mimicing how the API/HTTPClients seem to named?, we could have a:

```suggestion
			b, err := c.GetBundle(url)
```

Code after:
			return nil
		}, backoff.WithMaxRetries(bo, 3))

		// bail if RESTWithNext errored out
		if err != nil {
			return nil, err
		}
	}

	if len(attestations) == 0 {
		return nil, newErrNoAttestations(name, digest)
	}

	if len(attestations) > limit {
		return attestations[:limit], nil
	}

	return attestations, nil
}

func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([]*Attestation, error) {
	fetched := make([]*Attestation, len(attestations))
	g := errgroup.Group{}
	for i, a := range attestations {
		g.Go(func() error {
			if a.Bundle == nil && a.BundleURL == "" {
				return fmt.Errorf("attestation has no bundle or bundle URL")
			}

			// for now, we fallback to the bundle field if the bundle URL is empty
			if a.BundleURL == "" {
				c.logger.VerbosePrintf("Bundle URL is empty. Falling back to bundle field\n\n")
				fetched[i] = &Attestation{
					Bundle: a.Bundle,
				}
				return nil
			}

			// otherwise fetch the bundle with the provided URL
			b, err := c.GetBundle(a.BundleURL)
			if err != nil {
				return fmt.Errorf("failed to fetch bundle with URL: %w", err)
			}
			fetched[i] = &Attestation{
				Bundle: b,
			}
			return nil
		})
	}

	if err := g.Wait(); err != nil {
Repository: cli/cli
File: pkg/cmd/cache/delete/delete.go (Go)

Code before:
			); err != nil {
				return err
			}

			if !opts.DeleteAll && opts.SucceedOnNoCaches {
				return cmdutil.FlagErrorf("--succeed-on-no-caches must be used in conjunction with --all")
			}

			if !opts.DeleteAll && len(args) == 0 {
				return cmdutil.FlagErrorf("must provide either cache id, cache key, or use --all")
			}

			if len(args) == 1 {
				opts.Identifier = args[0]
			}

			if runF != nil {
				return runF(opts)
			}

			return deleteRun(opts)
		},
	}

	cmd.Flags().BoolVarP(&opts.DeleteAll, "all", "a", false, "Delete all caches")
	cmd.Flags().BoolVarP(&opts.SucceedOnNoCaches, "succeed-on-no-caches", "s", false, "Return exit code 0 if no caches found. Must be used in conjunction with `--all`")

	return cmd
}

func deleteRun(opts *DeleteOptions) error {
	httpClient, err := opts.HttpClient()
	if err != nil {
		return fmt.Errorf("failed to create http client: %w", err)
	}
	client := api.NewClientFromHTTP(httpClient)

	repo, err := opts.BaseRepo()
	if err != nil {
		return fmt.Errorf("failed to determine base repo: %w", err)
	}

	var toDelete []string
	if opts.DeleteAll {
		opts.IO.StartProgressIndicator()
		caches, err := shared.GetCaches(client, repo, shared.GetCachesOptions{Limit: -1})
		opts.IO.StopProgressIndicator()
		if err != nil {
			return err
		}
		if len(caches.ActionsCaches) == 0 {

Review comment (suggestion):
I don't think we should include an abbreviated flag syntax for this. I'd prefer to force people to type it out so there's less likelihood of someone accidentally messing this up 😅 

Code after:
			); err != nil {
				return err
			}

			if !opts.DeleteAll && opts.SucceedOnNoCaches {
				return cmdutil.FlagErrorf("--succeed-on-no-caches must be used in conjunction with --all")
			}

			if !opts.DeleteAll && len(args) == 0 {
				return cmdutil.FlagErrorf("must provide either cache id, cache key, or use --all")
			}

			if len(args) == 1 {
				opts.Identifier = args[0]
			}

			if runF != nil {
				return runF(opts)
			}

			return deleteRun(opts)
		},
	}

	cmd.Flags().BoolVarP(&opts.DeleteAll, "all", "a", false, "Delete all caches")
	cmd.Flags().BoolVar(&opts.SucceedOnNoCaches, "succeed-on-no-caches", false, "Return exit code 0 if no caches found. Must be used in conjunction with `--all`")

	return cmd
}

func deleteRun(opts *DeleteOptions) error {
	httpClient, err := opts.HttpClient()
	if err != nil {
		return fmt.Errorf("failed to create http client: %w", err)
	}
	client := api.NewClientFromHTTP(httpClient)

	repo, err := opts.BaseRepo()
	if err != nil {
		return fmt.Errorf("failed to determine base repo: %w", err)
	}

	var toDelete []string
	if opts.DeleteAll {
		opts.IO.StartProgressIndicator()
		caches, err := shared.GetCaches(client, repo, shared.GetCachesOptions{Limit: -1})
		opts.IO.StopProgressIndicator()
		if err != nil {
			return err
		}
		if len(caches.ActionsCaches) == 0 {
Repository: cli/cli
File: pkg/cmd/cache/delete/delete_test.go (Go)

Code before:
					httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"),
					httpmock.JSONResponse(shared.CachePayload{
						ActionsCaches: []shared.Cache{},
						TotalCount:    0,
					}),
				)
			},
			tty:        true,
			wantErr:    false,
			wantStdout: "✓ No caches to delete\n",
		},
		{
			name: "no caches to delete when deleting all but succeed on no cache non-tty",
			opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true},
			stubs: func(reg *httpmock.Registry) {
				reg.Register(
					httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"),
					httpmock.JSONResponse(shared.CachePayload{
						ActionsCaches: []shared.Cache{},
						TotalCount:    0,
					}),
				)
			},
			tty:        false,
			wantErr:    false,
			wantStdout: "✓ No caches to delete\n",
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			reg := &httpmock.Registry{}
			if tt.stubs != nil {
				tt.stubs(reg)
			}
			tt.opts.HttpClient = func() (*http.Client, error) {
				return &http.Client{Transport: reg}, nil
			}
			ios, _, stdout, stderr := iostreams.Test()
			ios.SetStdoutTTY(tt.tty)
			ios.SetStdinTTY(tt.tty)
			ios.SetStderrTTY(tt.tty)
			tt.opts.IO = ios
			tt.opts.BaseRepo = func() (ghrepo.Interface, error) {
				return ghrepo.New("OWNER", "REPO"), nil
			}
			defer reg.Verify(t)

			err := deleteRun(&tt.opts)
			if tt.wantErr {
				if tt.wantErrMsg != "" {

Review comment (suggestion):
Ah, I missed this in your comment on my PR. This should be empty, as we are in non-tty. The test was written correctly before, it was the _code_ that needed to change so this test case would pass. My bad for not catching it on the diff 😅 

```suggestion
			wantStdout: "",
```

Code after:
					httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"),
					httpmock.JSONResponse(shared.CachePayload{
						ActionsCaches: []shared.Cache{},
						TotalCount:    0,
					}),
				)
			},
			tty:        true,
			wantErr:    false,
			wantStdout: "✓ No caches to delete\n",
		},
		{
			name: "no caches to delete when deleting all but succeed on no cache non-tty",
			opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true},
			stubs: func(reg *httpmock.Registry) {
				reg.Register(
					httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"),
					httpmock.JSONResponse(shared.CachePayload{
						ActionsCaches: []shared.Cache{},
						TotalCount:    0,
					}),
				)
			},
			tty:        false,
			wantErr:    false,
			wantStdout: "",
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			reg := &httpmock.Registry{}
			if tt.stubs != nil {
				tt.stubs(reg)
			}
			tt.opts.HttpClient = func() (*http.Client, error) {
				return &http.Client{Transport: reg}, nil
			}
			ios, _, stdout, stderr := iostreams.Test()
			ios.SetStdoutTTY(tt.tty)
			ios.SetStdinTTY(tt.tty)
			ios.SetStderrTTY(tt.tty)
			tt.opts.IO = ios
			tt.opts.BaseRepo = func() (ghrepo.Interface, error) {
				return ghrepo.New("OWNER", "REPO"), nil
			}
			defer reg.Verify(t)

			err := deleteRun(&tt.opts)
			if tt.wantErr {
				if tt.wantErrMsg != "" {