ronantakizawa/codereview-bench huggingface dataset:
Repository: moby/moby
File: daemon/list.go (Go)

Code before:
	if err != nil {
		return nil, err
	}
	numContainers := len(containerList)

	// shortcut if there are no containers
	if numContainers == 0 {
		return []*containertypes.Summary{}, nil
	}

	// Get the info for each container in the list; this can be slow so we
	// dispatch a set number of worker goroutines to do the jobs. We choose
	// log2(numContainers) workers to avoid creating too many goroutines
	// for large number of containers.
	numWorkers := int(math.Log2(float64(numContainers)))
	if numWorkers < 1 {
		numWorkers = 1
	}

	resultsMut := sync.Mutex{}
	results := []*containertypes.Summary{}

	// Pre-fill results slice so we can later insert at specific indexes
	for range numContainers {
		results = append(results, nil)
	}

	g, ctx := errgroup.WithContext(ctx)
	g.SetLimit(numWorkers)

	for _, ctr := range containerList {
		stop := false
		switch includeContainerInList(&ctr, filter) {
		case excludeContainer:
			continue
		case stopIteration:
			stop = true
		case includeContainer:
			idx := filter.idx
			filter.idx++

			g.Go(func() error {
				// refresh the container image info (in case the image changed in
				// the repository)
				newC := daemon.refreshImage(ctx, &ctr)

				// get the image size (calculation is slow)
				if filter.Size {
					var err error
					newC.SizeRw, newC.SizeRootFs, err = daemon.imageService.GetContainerLayerSize(ctx, newC.ID)
					if err != nil {

Review comment (suggestion):
```suggestion
	results := make([]*containertypes.Summary{}, len(numContainers)
```

Code after:
	if err != nil {
		return nil, err
	}
	numContainers := len(containerList)

	// shortcut if there are no containers
	if numContainers == 0 {
		return []*containertypes.Summary{}, nil
	}

	// Get the info for each container in the list; this can be slow so we
	// dispatch a set number of worker goroutines to do the jobs. We choose
	// log2(numContainers) workers to avoid creating too many goroutines
	// for large number of containers.
	numWorkers := int(math.Log2(float64(numContainers)))
	if numWorkers < 1 {
		numWorkers = 1
	}

	resultsMut := sync.Mutex{}
	results := make([]*containertypes.Summary, numContainers)

	g, ctx := errgroup.WithContext(ctx)
	g.SetLimit(numWorkers)

	for i := range containerList {
		currentContainer := &containerList[i]
		stop := false
		switch includeContainerInList(currentContainer, filter) {
		case excludeContainer:
			continue
		case stopIteration:
			stop = true
		case includeContainer:
			idx := filter.idx
			filter.idx++

			g.Go(func() error {
				// refresh the container image info (in case the image changed in
				// the repository)
				newC := daemon.refreshImage(ctx, currentContainer)

				// get the image size (calculation is slow)
				if filter.Size {
					var err error
					newC.SizeRw, newC.SizeRootFs, err = daemon.imageService.GetContainerLayerSize(ctx, newC.ID)
					if err != nil {
						return err
					}
				}
Repository: moby/moby
File: daemon/list_test.go (Go)

Code before:
package daemon

import (
	"context"
	"fmt"
	"math/rand"
	"os"
	"path/filepath"
	"testing"
	"time"

	containertypes "github.com/docker/docker/api/types/container"
	"github.com/docker/docker/api/types/filters"
	"github.com/docker/docker/container"
	"github.com/docker/docker/image"
	"github.com/google/uuid"
	"github.com/opencontainers/go-digest"
	"gotest.tools/v3/assert"
	is "gotest.tools/v3/assert/cmp"
)

var (
	root    string
	randGen *rand.Rand
)

func TestMain(m *testing.M) {
	var err error
	root, err = os.MkdirTemp("", "docker-container-test-")
	if err != nil {
		panic(err)
	}
	defer os.RemoveAll(root)

	// Use constant seed for reproducibility
	randGen = rand.New(rand.NewSource(1982731263716))

	os.Exit(m.Run())
}

// This sets up a container with a name so that name filters
// work against it. It takes in a pointer to Daemon so that
// minor operations are not repeated by the caller
func setupContainerWithName(t *testing.T, name string, daemon *Daemon) *container.Container {
	t.Helper()
	var (
		id              = uuid.New().String()
		computedImageID = image.ID(digest.FromString(id))
		cRoot           = filepath.Join(root, id)

Review comment (suggestion):
IMO, we shouldn't make that global and should keep it local to the test.

As soon as we have parallel tests sampling the random source (or maybe we have that already), the output won't be consistent because goroutines will sample it in a non-deterministic order.

Code after:
package daemon

import (
	"context"
	"fmt"
	"os"
	"path/filepath"
	"testing"
	"time"

	containertypes "github.com/docker/docker/api/types/container"
	"github.com/docker/docker/api/types/filters"
	"github.com/docker/docker/container"
	"github.com/docker/docker/image"
	"github.com/google/uuid"
	"github.com/opencontainers/go-digest"
	"gotest.tools/v3/assert"
	is "gotest.tools/v3/assert/cmp"
)

var root string

func TestMain(m *testing.M) {
	var err error
	root, err = os.MkdirTemp("", "docker-container-test-")
	if err != nil {
		panic(err)
	}
	defer os.RemoveAll(root)
	os.Exit(m.Run())
}

// This sets up a container with a name so that name filters
// work against it. It takes in a pointer to Daemon so that
// minor operations are not repeated by the caller
func setupContainerWithName(t *testing.T, name string, daemon *Daemon) *container.Container {
	t.Helper()
	var (
		id              = uuid.New().String()
		computedImageID = image.ID(digest.FromString(id))
		cRoot           = filepath.Join(root, id)
	)
	if err := os.MkdirAll(cRoot, 0o755); err != nil {
		t.Fatal(err)
	}

	c := container.NewBaseContainer(id, cRoot)
	// these are for passing includeContainerInList
	if name[0] != '/' {
Repository: moby/moby
File: client/image_load.go (Go)

Code before:
package client // import "github.com/docker/docker/client"

import (
	"context"
	"fmt"
	"io"
	"net/http"
	"net/url"

	"github.com/docker/docker/api/types/image"
	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
)

// ImageLoadOption is a type representing functional options for the image load operation.
type ImageLoadOption interface {
	Apply(*imageLoadOpts) error
}
type imageLoadOptionFunc func(opt *imageLoadOpts) error

func (f imageLoadOptionFunc) Apply(o *imageLoadOpts) error {
	return f(o)
}

type imageLoadOpts struct {
	apiOptions image.LoadOptions
}

// ImageLoadWithQuiet sets the quiet option for the image load operation.
func ImageLoadWithQuiet(quiet bool) ImageLoadOption {
	return imageLoadOptionFunc(func(opt *imageLoadOpts) error {
		opt.apiOptions.Quiet = quiet
		return nil
	})
}

// ImageLoadWithPlatforms sets the platforms to be loaded from the image.
func ImageLoadWithPlatforms(platforms ...ocispec.Platform) ImageLoadOption {
	return imageLoadOptionFunc(func(opt *imageLoadOpts) error {
		if opt.apiOptions.Platforms != nil {
			return fmt.Errorf("platforms already set to %v", opt.apiOptions.Platforms)
		}
		opt.apiOptions.Platforms = platforms

Review comment (suggestion):
Small nit; not sure what we did for the other ones, but perhaps we should choose to put the options in a separate file (also if we at some point would decide to split "clients" to smaller ones as we were contemplating); e.g. `image_load_options.go`

That way, if things would be split, we can make these a lateral move to a sub-package (and an alias in return)


Code after:
package client // import "github.com/docker/docker/client"

import (
	"context"
	"io"
	"net/http"
	"net/url"

	"github.com/docker/docker/api/types/image"
)

// ImageLoad loads an image in the docker host from the client host.
// It's up to the caller to close the io.ReadCloser in the
// ImageLoadResponse returned by this function.
//
// Platform is an optional parameter that specifies the platform to load from
// the provided multi-platform image. This is only has effect if the input image
// is a multi-platform image.
func (cli *Client) ImageLoad(ctx context.Context, input io.Reader, loadOpts ...ImageLoadOption) (image.LoadResponse, error) {
	var opts imageLoadOpts
	for _, opt := range loadOpts {
		if err := opt.Apply(&opts); err != nil {
			return image.LoadResponse{}, err
		}
	}

	query := url.Values{}
	query.Set("quiet", "0")
	if opts.apiOptions.Quiet {
		query.Set("quiet", "1")
	}
	if len(opts.apiOptions.Platforms) > 0 {
		if err := cli.NewVersionError(ctx, "1.48", "platform"); err != nil {
			return image.LoadResponse{}, err
		}

		p, err := encodePlatforms(opts.apiOptions.Platforms...)
		if err != nil {
			return image.LoadResponse{}, err
		}
		query["platform"] = p
	}
Repository: moby/moby
File: daemon/start_linux.go (Go)

Code before:
)

// initializeCreatedTask performs any initialization that needs to be done to
// prepare a freshly-created task to be started.
func (daemon *Daemon) initializeCreatedTask(
	ctx context.Context,
	cfg *config.Config,
	tsk types.Task,
	ctr *container.Container,
	spec *specs.Spec,
) error {
	if ctr.Config.NetworkDisabled {
		return nil
	}
	nspath, ok := oci.NamespacePath(spec, specs.NetworkNamespace)
	if ok && nspath == "" { // the runtime has been instructed to create a new network namespace for tsk.
		sb, err := daemon.netController.GetSandbox(ctr.ID)
		if err != nil {
			return errdefs.System(err)
		}
		if err := sb.SetKey(ctx, fmt.Sprintf("/proc/%d/ns/net", tsk.Pid())); err != nil {
			return errdefs.System(err)
		}
	}
	if err := daemon.allocateNetwork(ctx, cfg, ctr); err != nil {
		return fmt.Errorf("failed to set up container networking: %w", err)
	}
	return nil
}


Review comment (bug):
The test failure should be enough to avoid regressions. But, as these are both in `package daemon`, could put the error string in a const to make it clear where the message is used?

Code after:
)

// initializeCreatedTask performs any initialization that needs to be done to
// prepare a freshly-created task to be started.
func (daemon *Daemon) initializeCreatedTask(
	ctx context.Context,
	cfg *config.Config,
	tsk types.Task,
	ctr *container.Container,
	spec *specs.Spec,
) error {
	if ctr.Config.NetworkDisabled {
		return nil
	}
	nspath, ok := oci.NamespacePath(spec, specs.NetworkNamespace)
	if ok && nspath == "" { // the runtime has been instructed to create a new network namespace for tsk.
		sb, err := daemon.netController.GetSandbox(ctr.ID)
		if err != nil {
			return errdefs.System(err)
		}
		if err := sb.SetKey(ctx, fmt.Sprintf("/proc/%d/ns/net", tsk.Pid())); err != nil {
			return errdefs.System(err)
		}
	}
	if err := daemon.allocateNetwork(ctx, cfg, ctr); err != nil {
		return fmt.Errorf("%s: %w", errSetupNetworking, err)
	}
	return nil
}
Repository: moby/moby
File: integration/internal/network/ops.go (Go)

Code before:

// WithConfigOnly sets the ConfigOnly flag in the create network request
func WithConfigOnly(co bool) func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.ConfigOnly = co
	}
}

// WithConfigFrom sets the ConfigOnly flag in the create network request
func WithConfigFrom(name string) func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.ConfigFrom = &network.ConfigReference{Network: name}
	}
}

// WithAttachable sets Attachable flag on the create network request
func WithAttachable() func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Attachable = true
	}
}

// WithScopeSwarm gives the network swarm scope.
func WithScopeSwarm() func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Scope = scope.Swarm
	}
}

// WithMacvlan sets the network as macvlan with the specified parent
func WithMacvlan(parent string) func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Driver = "macvlan"
		if parent != "" {
			n.Options = map[string]string{
				"parent": parent,
			}
		}
	}
}

// WithMacvlanPassthru sets the network as macvlan with the specified parent in passthru mode
func WithMacvlanPassthru(parent string) func(options *network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Driver = "macvlan"
		n.Options = map[string]string{
			"macvlan_mode": "passthru",
		}
		if parent != "" {
			n.Options["parent"] = parent
		}

Review comment (suggestion):
Mostly nit, because this is internal and test-utils, but wondered if `WithScope(name)` would be useful in case we'd need other scopes

Code after:
// WithConfigOnly sets the ConfigOnly flag in the create network request
func WithConfigOnly(co bool) func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.ConfigOnly = co
	}
}

// WithConfigFrom sets the ConfigOnly flag in the create network request
func WithConfigFrom(name string) func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.ConfigFrom = &network.ConfigReference{Network: name}
	}
}

// WithAttachable sets Attachable flag on the create network request
func WithAttachable() func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Attachable = true
	}
}

// WithScope sets the network scope.
func WithScope(s string) func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Scope = s
	}
}

// WithMacvlan sets the network as macvlan with the specified parent
func WithMacvlan(parent string) func(*network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Driver = "macvlan"
		if parent != "" {
			n.Options = map[string]string{
				"parent": parent,
			}
		}
	}
}

// WithMacvlanPassthru sets the network as macvlan with the specified parent in passthru mode
func WithMacvlanPassthru(parent string) func(options *network.CreateOptions) {
	return func(n *network.CreateOptions) {
		n.Driver = "macvlan"
		n.Options = map[string]string{
			"macvlan_mode": "passthru",
		}
		if parent != "" {
			n.Options["parent"] = parent
		}
	}
Repository: moby/moby
File: internal/platform/platform_linux_test.go (Go)

Code before:
package platform

import (
	"testing"

	"gotest.tools/v3/assert"
	"gotest.tools/v3/assert/cmp"
)

func TestParsePossibleCPUs(t *testing.T) {
	tests := []struct {
		name     string
		input    string
		expected []int
	}{
		{
			name:     "Continuous Range",
			input:    "0-3",
			expected: []int{0, 1, 2, 3},
		},
		{
			name:     "Non-Continuous Range",
			input:    "0-2,4,6-7",
			expected: []int{0, 1, 2, 4, 6, 7},
		},
		{
			name:     "Single CPU",
			input:    "5",
			expected: []int{5},
		},
		{
			name:     "Empty Input",

Review comment (suggestion):
```suggestion
	is "gotest.tools/v3/assert/cmp"
```

Code after:
package platform

import (
	"testing"

	"gotest.tools/v3/assert"
	is "gotest.tools/v3/assert/cmp"
)

func TestParsePossibleCPUs(t *testing.T) {
	tests := []struct {
		name     string
		input    string
		expected []int
	}{
		{
			name:     "Continuous Range",
			input:    "0-3",
			expected: []int{0, 1, 2, 3},
		},
		{
			name:     "Non-Continuous Range",
			input:    "0-2,4,6-7",
			expected: []int{0, 1, 2, 4, 6, 7},
		},
		{
			name:     "Single CPU",
			input:    "5",
			expected: []int{5},
		},
		{
			name:     "Empty Input",
Repository: moby/moby
File: internal/platform/platform_linux_test.go (Go)

Code before:
		{
			name:     "Single CPU",
			input:    "5",
			expected: []int{5},
		},
		{
			name:     "Empty Input",
			input:    "",
			expected: nil,
		},
		{
			name:     "Invalid Range",
			input:    "0-2,invalid",
			expected: nil,
		},
		{
			name:     "Malformed Range",
			input:    "0-2-3",
			expected: nil,
		},
	}

	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			result := parsePossibleCPUs(test.input)
			assert.Assert(t, cmp.DeepEqual(result, test.expected), "Expected %v but got %v", test.expected, result)
		})
	}
}


Review comment (suggestion):
```suggestion
			assert.Assert(t, is.DeepEqual(result, test.expected), "Expected %v but got %v", test.expected, result)
```

Code after:
		{
			name:     "Single CPU",
			input:    "5",
			expected: []int{5},
		},
		{
			name:     "Empty Input",
			input:    "",
			expected: nil,
		},
		{
			name:     "Invalid Range",
			input:    "0-2,invalid",
			expected: nil,
		},
		{
			name:     "Malformed Range",
			input:    "0-2-3",
			expected: nil,
		},
	}

	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			result := parsePossibleCPUs(test.input)
			assert.Assert(t, is.DeepEqual(result, test.expected), "Expected %v but got %v", test.expected, result)
		})
	}
}
Repository: moby/moby
File: client/image_inspect.go (Go)

Code before:
	"context"
	"encoding/json"
	"fmt"
	"io"
	"net/url"

	"github.com/docker/docker/api/types/image"
)

// ImageInspect returns the image information.
func (cli *Client) ImageInspect(ctx context.Context, imageID string, inspectOpts ...ImageInspectOption) (image.InspectResponse, error) {
	if imageID == "" {
		return image.InspectResponse{}, objectNotFoundError{object: "image", id: imageID}
	}

	var opts imageInspectOpts
	for _, opt := range inspectOpts {
		if err := opt.Apply(&opts); err != nil {
			return image.InspectResponse{}, fmt.Errorf("error applying image inspect option: %w", err)
		}
	}

	if opts.apiOptions.Manifests && opts.apiOptions.Platform != nil {
		return image.InspectResponse{}, fmt.Errorf("manifests and platform options cannot both be set")
	}

	query := url.Values{}
	if opts.apiOptions.Manifests {
		if err := cli.NewVersionError(ctx, "1.48", "manifests"); err != nil {
			return image.InspectResponse{}, err
		}
		query.Set("manifests", "1")
	}

	if opts.apiOptions.Platform != nil {
		if err := cli.NewVersionError(ctx, "1.49", "platform"); err != nil {
			return image.InspectResponse{}, err
		}
		platform, err := encodePlatform(opts.apiOptions.Platform)
		if err != nil {
			return image.InspectResponse{}, err
		}
		query.Set("platform", platform)
	}

	resp, err := cli.get(ctx, "/images/"+imageID+"/json", query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return image.InspectResponse{}, err
	}


Review comment (refactor):
If we _do_ want to keep the error, we should probably remove this check in the client, and leave it to the API server to return an error

Code after:
	"context"
	"encoding/json"
	"fmt"
	"io"
	"net/url"

	"github.com/docker/docker/api/types/image"
)

// ImageInspect returns the image information.
func (cli *Client) ImageInspect(ctx context.Context, imageID string, inspectOpts ...ImageInspectOption) (image.InspectResponse, error) {
	if imageID == "" {
		return image.InspectResponse{}, objectNotFoundError{object: "image", id: imageID}
	}

	var opts imageInspectOpts
	for _, opt := range inspectOpts {
		if err := opt.Apply(&opts); err != nil {
			return image.InspectResponse{}, fmt.Errorf("error applying image inspect option: %w", err)
		}
	}

	query := url.Values{}
	if opts.apiOptions.Manifests {
		if err := cli.NewVersionError(ctx, "1.48", "manifests"); err != nil {
			return image.InspectResponse{}, err
		}
		query.Set("manifests", "1")
	}

	if opts.apiOptions.Platform != nil {
		if err := cli.NewVersionError(ctx, "1.49", "platform"); err != nil {
			return image.InspectResponse{}, err
		}
		platform, err := encodePlatform(opts.apiOptions.Platform)
		if err != nil {
			return image.InspectResponse{}, err
		}
		query.Set("platform", platform)
	}

	resp, err := cli.get(ctx, "/images/"+imageID+"/json", query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return image.InspectResponse{}, err
	}

	buf := opts.raw
	if buf == nil {
		buf = &bytes.Buffer{}
	}
Repository: moby/moby
File: daemon/network.go (Go)

Code before:
		libnetwork.CreateOptionPortMapping(publishedPorts),
		libnetwork.CreateOptionExposedPorts(exposedPorts),
	}, nil
}

// getPortMapInfo retrieves the current port-mapping programmed for the given sandbox
func getPortMapInfo(sb *libnetwork.Sandbox) nat.PortMap {
	pm := nat.PortMap{}
	if sb == nil {
		return pm
	}

	for _, ep := range sb.Endpoints() {
		getEndpointPortMapInfo(pm, ep)
	}
	return pm
}

func getEndpointPortMapInfo(pm nat.PortMap, ep *libnetwork.Endpoint) {
	driverInfo, _ := ep.DriverInfo()
	if driverInfo == nil {
		// It is not an error for epInfo to be nil
		return
	}

	if containertypes.NetworkMode(ep.Network()).IsBridge() {
		if expData, ok := driverInfo[netlabel.ExposedPorts]; ok {
			if exposedPorts, ok := expData.([]lntypes.TransportPort); ok {
				for _, tp := range exposedPorts {
					natPort, err := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port)))
					if err != nil {
						return
					}
					pm[natPort] = nil
				}
			}
		}
	}

	mapData, ok := driverInfo[netlabel.PortMap]
	if !ok {
		return
	}

	if portMapping, ok := mapData.([]lntypes.PortBinding); ok {
		for _, pp := range portMapping {
			// Use an empty string for the host port if there's no port assigned.
			natPort, err := nat.NewPort(pp.Proto.String(), strconv.Itoa(int(pp.Port)))
			if err != nil {
				return
			}

Review comment (bug):
To be honest, I feel like this is a bit crappy as this is going to cause issues when we'll add support for DNS-based links to custom networks. But this is the shortest path to get this bug fixed in v28.0.2.



Code after:
		libnetwork.CreateOptionPortMapping(publishedPorts),
		libnetwork.CreateOptionExposedPorts(exposedPorts),
	}, nil
}

// getPortMapInfo retrieves the current port-mapping programmed for the given sandbox
func getPortMapInfo(sb *libnetwork.Sandbox) nat.PortMap {
	pm := nat.PortMap{}
	if sb == nil {
		return pm
	}

	for _, ep := range sb.Endpoints() {
		getEndpointPortMapInfo(pm, ep)
	}
	return pm
}

func getEndpointPortMapInfo(pm nat.PortMap, ep *libnetwork.Endpoint) {
	driverInfo, _ := ep.DriverInfo()
	if driverInfo == nil {
		// It is not an error for epInfo to be nil
		return
	}

	if expData, ok := driverInfo[netlabel.ExposedPorts]; ok {
		if exposedPorts, ok := expData.([]lntypes.TransportPort); ok {
			for _, tp := range exposedPorts {
				natPort, err := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port)))
				if err != nil {
					log.G(context.TODO()).Errorf("invalid exposed port %s: %v", tp.String(), err)
					continue
				}
				if _, ok := pm[natPort]; !ok {
					pm[natPort] = nil
				}
			}
		}
	}

	mapData, ok := driverInfo[netlabel.PortMap]
	if !ok {
		return
	}

	if portMapping, ok := mapData.([]lntypes.PortBinding); ok {
		for _, pp := range portMapping {
			// Use an empty string for the host port if there's no port assigned.
			natPort, err := nat.NewPort(pp.Proto.String(), strconv.Itoa(int(pp.Port)))
			if err != nil {
				log.G(context.TODO()).Errorf("invalid port binding %s: %v", pp, err)