ronantakizawa/codereview-bench huggingface dataset:
Repository: moby/moby
File: integration/container/create_test.go (Go)

Code before:
				&container.HostConfig{},
				&network.NetworkingConfig{},
				nil,
				"",
			)
			assert.Check(t, is.ErrorContains(err, tc.expectedError))
			assert.Check(t, is.ErrorType(err, cerrdefs.IsNotFound))
		})
	}
}

func TestCreateByImageID(t *testing.T) {
	ctx := setupTest(t)
	apiClient := testEnv.APIClient()

	img, err := apiClient.ImageInspect(ctx, "busybox")
	assert.NilError(t, err)

	imgIDWithAlgorithm := img.ID
	imgID, _ := strings.CutPrefix(imgIDWithAlgorithm, "sha256:")
	imgShortID := stringid.TruncateID(imgID)

	testCases := []struct {
		doc             string
		image           string
		expectedErrType func(error) bool
		expectedErr     string
	}{
		{
			doc:   "image ID with algorithm",
			image: imgIDWithAlgorithm,
		},
		{
			// test case for https://github.com/moby/moby/issues/20972
			doc:   "image ID without algorithm",
			image: imgID,
		},
		{
			doc:   "image short-ID",
			image: imgShortID,
		},
		{
			doc:             "image with ID and algorithm as tag",
			image:           "busybox:" + imgIDWithAlgorithm,
			expectedErrType: cerrdefs.IsInvalidArgument,
			expectedErr:     "Error response from daemon: invalid reference format",
		},
		{
			doc:             "image with ID as tag",
			image:           "busybox:" + imgID,
			expectedErrType: cerrdefs.IsNotFound,

Review comment (bug):
FWIW I was slightly on the fence if this was _too much_ defence-in-depth; I initially wrote it for debugging, but then thought "doesn't hurt either" so I kept these.

Code after:
				&container.HostConfig{},
				&network.NetworkingConfig{},
				nil,
				"",
			)
			assert.Check(t, is.ErrorContains(err, tc.expectedError))
			assert.Check(t, is.ErrorType(err, cerrdefs.IsNotFound))
		})
	}
}

func TestCreateByImageID(t *testing.T) {
	ctx := setupTest(t)
	apiClient := testEnv.APIClient()

	img, err := apiClient.ImageInspect(ctx, "busybox")
	assert.NilError(t, err)

	imgIDWithAlgorithm := img.ID
	assert.Assert(t, imgIDWithAlgorithm != "")

	imgID, _ := strings.CutPrefix(img.ID, "sha256:")
	assert.Assert(t, imgID != "")

	imgShortID := stringid.TruncateID(img.ID)
	assert.Assert(t, imgShortID != "")

	testCases := []struct {
		doc             string
		image           string
		expectedErrType func(error) bool
		expectedErr     string
	}{
		{
			doc:   "image ID with algorithm",
			image: imgIDWithAlgorithm,
		},
		{
			// test case for https://github.com/moby/moby/issues/20972
			doc:   "image ID without algorithm",
			image: imgID,
		},
		{
			doc:   "image short-ID",
			image: imgShortID,
		},
		{
			doc:             "image with ID and algorithm as tag",
			image:           "busybox:" + imgIDWithAlgorithm,
			expectedErrType: cerrdefs.IsInvalidArgument,
			expectedErr:     "Error response from daemon: invalid reference format",
Repository: moby/moby
File: daemon/volumes.go (Go)

Code before:
				mp.SkipMountpointCreation = true
			}

			if !mp.RW && defaultReadOnlyNonRecursive {
				if mp.Spec.BindOptions == nil {
					mp.Spec.BindOptions = &mounttypes.BindOptions{}
				}
				mp.Spec.BindOptions.ReadOnlyNonRecursive = true
			}
		}

		if mp.Type == mounttypes.TypeImage {
			img, err := daemon.imageService.GetImage(ctx, mp.Source, backend.GetImageOpts{})
			if err != nil {
				return err
			}

			rwLayerOpts := &layer.CreateRWLayerOpts{
				StorageOpt: container.HostConfig.StorageOpt,
			}

			// Include the destination in the layer name to make it unique for each mount point and container.
			// This makes sure that the same image can be mounted multiple times with different destinations.
			// Hash the destination to create a safe, unique identifier
			destHash := sha256.Sum256([]byte(container.ID + "src=" + mp.Source + "dst=" + mp.Destination))
			layerName := hex.EncodeToString(destHash[:])
			layer, err := daemon.imageService.CreateLayerFromImage(img, layerName, rwLayerOpts)
			if err != nil {
				return err
			}
			metadata, err := layer.Metadata()
			if err != nil {
				return err
			}

			path, err := layer.Mount("")
			if err != nil {
				return err
			}

			if metadata["ID"] != "" {
				mp.ID = metadata["ID"]
			}

			mp.Name = mp.Spec.Source
			mp.Spec.Source = img.ID().String()
			mp.Source = path
			mp.Layer = layer
			mp.RW = false
		}


Review comment (suggestion):
Would we need to correlate the ID with these anywhere? Considering if (e.g.) base64 encoding or something like that would work instead of a cryptographic hash.

What are the constraints on these names? (is this path on disk, or otherwise?)

Code after:
			}

			if !mp.RW && defaultReadOnlyNonRecursive {
				if mp.Spec.BindOptions == nil {
					mp.Spec.BindOptions = &mounttypes.BindOptions{}
				}
				mp.Spec.BindOptions.ReadOnlyNonRecursive = true
			}
		}

		if mp.Type == mounttypes.TypeImage {
			img, err := daemon.imageService.GetImage(ctx, mp.Source, backend.GetImageOpts{})
			if err != nil {
				return err
			}

			rwLayerOpts := &layer.CreateRWLayerOpts{
				StorageOpt: container.HostConfig.StorageOpt,
			}

			// Include the destination in the layer name to make it unique for each mount point and container.
			// This makes sure that the same image can be mounted multiple times with different destinations.
			// Hex encode the destination to create a safe, unique identifier
			layerName := hex.EncodeToString([]byte(container.ID + ",src=" + mp.Source + ",dst=" + mp.Destination))
			layer, err := daemon.imageService.CreateLayerFromImage(img, layerName, rwLayerOpts)
			if err != nil {
				return err
			}
			metadata, err := layer.Metadata()
			if err != nil {
				return err
			}

			path, err := layer.Mount("")
			if err != nil {
				return err
			}

			if metadata["ID"] != "" {
				mp.ID = metadata["ID"]
			}

			mp.Name = mp.Spec.Source
			mp.Spec.Source = img.ID().String()
			mp.Source = path
			mp.Layer = layer
			mp.RW = false
		}

		binds[mp.Destination] = true
		dereferenceIfExists(mp.Destination)
Repository: moby/moby
File: daemon/container/view.go (Go)

Code before:
		return nil
	}

	out := make(map[string][]string)
	for {
		item := iter.Next()
		if item == nil {
			break
		}
		assoc := item.(nameAssociation)
		out[assoc.containerID] = append(out[assoc.containerID], assoc.name)
	}

	return out
}

// transform maps a (deep) copied Container object to what queries need.
// A lock on the Container is not held because these are immutable deep copies.
func (v *View) transform(ctr *Container) *Snapshot {
	health := container.NoHealthcheck
	if ctr.Health != nil {
		health = ctr.Health.Status()
	}

	var healthSummary *container.HealthSummary
	if versions.GreaterThanOrEqualTo(api.DefaultVersion, "1.51") {
		failingStreak := 0
		if ctr.Health != nil {
			failingStreak = ctr.Health.FailingStreak
		}
		healthSummary = &container.HealthSummary{
			Status:        health,
			FailingStreak: failingStreak,
		}
	}

	snapshot := &Snapshot{
		Summary: container.Summary{
			ID:      ctr.ID,
			Names:   v.getNames(ctr.ID),
			ImageID: ctr.ImageID.String(),
			Ports:   []container.Port{},
			Mounts:  ctr.GetMountPoints(),
			State:   ctr.State.StateString(),
			Status:  ctr.State.String(),
			Health:  healthSummary,
			Created: ctr.Created.Unix(),
		},
		CreatedAt:    ctr.Created,
		StartedAt:    ctr.StartedAt,
		Name:         ctr.Name,

Review comment (suggestion):
The API version check should be handled in the API server:

https://github.com/moby/moby/blob/0d189dd4e1a8c7a1e1a97d40ca6781766b8e44b7/daemon/server/router/container/container_routes.go#L106-L120

Code after:

	out := make(map[string][]string)
	for {
		item := iter.Next()
		if item == nil {
			break
		}
		assoc := item.(nameAssociation)
		out[assoc.containerID] = append(out[assoc.containerID], assoc.name)
	}

	return out
}

// transform maps a (deep) copied Container object to what queries need.
// A lock on the Container is not held because these are immutable deep copies.
func (v *View) transform(ctr *Container) *Snapshot {
	health := container.NoHealthcheck
	failingStreak := 0
	if ctr.Health != nil {
		health = ctr.Health.Status()
		failingStreak = ctr.Health.FailingStreak
	}

	healthSummary := &container.HealthSummary{
		Status:        health,
		FailingStreak: failingStreak,
	}

	snapshot := &Snapshot{
		Summary: container.Summary{
			ID:      ctr.ID,
			Names:   v.getNames(ctr.ID),
			ImageID: ctr.ImageID.String(),
			Ports:   []container.Port{},
			Mounts:  ctr.GetMountPoints(),
			State:   ctr.State.StateString(),
			Status:  ctr.State.String(),
			Health:  healthSummary,
			Created: ctr.Created.Unix(),
		},
		CreatedAt:    ctr.Created,
		StartedAt:    ctr.StartedAt,
		Name:         ctr.Name,
		Pid:          ctr.Pid,
		Managed:      ctr.Managed,
		ExposedPorts: make(nat.PortSet),
		PortBindings: make(nat.PortSet),
		Health:       health,
		Running:      ctr.Running,
		Paused:       ctr.Paused,
Repository: moby/moby
File: daemon/libnetwork/firewall_linux.go (Go)

Code before:
package libnetwork

import (
	"context"
	"errors"
	"fmt"
	"os"

	"github.com/containerd/log"
	"github.com/docker/docker/daemon/libnetwork/internal/nftables"
	"github.com/docker/docker/daemon/libnetwork/iptables"
)

const userChain = "DOCKER-USER"

func ValidateFirewallBackend(val string) error {
	switch val {
	case "", "iptables", "nftables":
		return nil
	}
	return fmt.Errorf(`firewall-backend %q is invalid, allowed values are "iptables" and "nftables"`, val)
}

func (c *Controller) selectFirewallBackend() error {
	// If explicitly configured to use iptables, don't consider nftables.
	if c.cfg.FirewallBackend == "iptables" {
		return nil
	}
	// If configured to use nftables, but it can't be initialised, return an error.
	if c.cfg.FirewallBackend == "nftables" {
		// Don't try to enable nftables if firewalld is running.
		if iptables.UsingFirewalld() {
			return errors.New("firewall-backend is set to nftables, but firewalld is running")
		}
		if err := nftables.Enable(); err != nil {
			return fmt.Errorf("firewall-backend is set to nftables: %v", err)
		}
		return nil
	}
	// Default to nftables if enabled by env-var.
	// (Used for integration tests that launch their own daemons in CI.)
	if os.Getenv("DOCKER_FIREWALL_BACKEND") == "nftables" {

Review comment (suggestion):
Just random thought; would it be useful if we persisted the result of the selection? Does it surface anywhere what was selected?


(This is where the functional arg / Option with an error return could've been good; possibly it could've done the selection) 

Code after:
package libnetwork

import (
	"context"
	"errors"
	"fmt"

	"github.com/containerd/log"
	"github.com/docker/docker/daemon/libnetwork/internal/nftables"
	"github.com/docker/docker/daemon/libnetwork/iptables"
)

const userChain = "DOCKER-USER"

func (c *Controller) selectFirewallBackend() error {
	// If explicitly configured to use iptables, don't consider nftables.
	if c.cfg.FirewallBackend == "iptables" {
		return nil
	}
	// If configured to use nftables, but it can't be initialised, return an error.
	if c.cfg.FirewallBackend == "nftables" {
		// Don't try to enable nftables if firewalld is running.
		if iptables.UsingFirewalld() {
			return errors.New("firewall-backend is set to nftables, but firewalld is running")
		}
		if err := nftables.Enable(); err != nil {
			return fmt.Errorf("firewall-backend is set to nftables: %v", err)
		}
		return nil
	}
	return nil
}

// Sets up the DOCKER-USER chain for each iptables version (IPv4, IPv6) that's
// enabled in the controller's configuration.
func (c *Controller) setupUserChains() {
	// There's no equivalent to DOCKER-USER in the nftables implementation.
	if nftables.Enabled() {
		return
	}

	setup := func() error {
Repository: moby/moby
File: integration/image/list_test.go (Go)

Code before:
package image

import (
	"fmt"
	"slices"
	"strings"
	"testing"
	"time"

	"github.com/docker/docker/api"
	containertypes "github.com/docker/docker/api/types/container"
	"github.com/docker/docker/api/types/filters"
	"github.com/docker/docker/api/types/image"
	"github.com/docker/docker/api/types/versions"
	"github.com/docker/docker/client"
	buildImage "github.com/docker/docker/integration/internal/build"
	"github.com/docker/docker/integration/internal/container"
	"github.com/docker/docker/internal/testutils/specialimage"
	"github.com/docker/docker/testutil"
	"github.com/docker/docker/testutil/daemon"
	"github.com/docker/docker/testutil/fakecontext"
	"github.com/google/go-cmp/cmp/cmpopts"
	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
	"gotest.tools/v3/assert"
	is "gotest.tools/v3/assert/cmp"
	"gotest.tools/v3/skip"
)

// Regression : #38171
func TestImagesFilterMultiReference(t *testing.T) {
	ctx := setupTest(t)

	client := testEnv.APIClient()

	name := strings.ToLower(t.Name())
	repoTags := []string{
		name + ":v1",
		name + ":v2",
		name + ":v3",
		name + ":v4",
	}

Review comment (suggestion):
I think we can just use the `build` package as it doesn't seem to clash with other imports.
```suggestion
	"github.com/docker/docker/integration/internal/build"
```

Code after:
package image

import (
	"fmt"
	"slices"
	"strings"
	"testing"
	"time"

	"github.com/docker/docker/api"
	containertypes "github.com/docker/docker/api/types/container"
	"github.com/docker/docker/api/types/filters"
	"github.com/docker/docker/api/types/image"
	"github.com/docker/docker/api/types/versions"
	"github.com/docker/docker/client"
	"github.com/docker/docker/integration/internal/container"
	"github.com/docker/docker/internal/testutils/specialimage"
	"github.com/docker/docker/testutil"
	"github.com/docker/docker/testutil/daemon"
	"github.com/google/go-cmp/cmp/cmpopts"
	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
	"gotest.tools/v3/assert"
	is "gotest.tools/v3/assert/cmp"
	"gotest.tools/v3/skip"
)

// Regression : #38171
func TestImagesFilterMultiReference(t *testing.T) {
	ctx := setupTest(t)

	client := testEnv.APIClient()

	name := strings.ToLower(t.Name())
	repoTags := []string{
		name + ":v1",
		name + ":v2",
		name + ":v3",
		name + ":v4",
	}

	for _, repoTag := range repoTags {
Repository: moby/moby
File: daemon/libnetwork/internal/resolvconf/resolvconf.go (Go)

Code before:
			return false, nil
		}
		return false, errors.Wrapf(err, "failed to read hash file %s", rcHashPath)
	}
	expected, err := digest.Parse(string(currRCHash))
	if err != nil {
		return false, errors.Wrapf(err, "failed to parse hash file %s", rcHashPath)
	}
	v := expected.Verifier()
	currRC, err := os.Open(rcPath)
	if err != nil {
		return false, errors.Wrapf(err, "failed to open %s to check for modifications", rcPath)
	}
	defer currRC.Close()
	if _, err := io.Copy(v, currRC); err != nil {
		return false, errors.Wrapf(err, "failed to hash %s to check for modifications", rcPath)
	}
	return !v.Verified(), nil
}

func (rc *ResolvConf) processLine(line string) {
	// Strip blank lines and comments.
	if line == "" || line[0] == '#' || line[0] == ';' {
		return
	}

	fields := strings.Fields(line)
	if len(fields) == 0 {
		return
	}

	switch fields[0] {
	case "nameserver":
		if len(fields) < 2 {
			return
		}
		if addr, err := netip.ParseAddr(fields[1]); err != nil {
			rc.md.InvalidNSs = append(rc.md.InvalidNSs, fields[1])
		} else {
			rc.nameServers = append(rc.nameServers, addr)
		}
	case "domain":
		// 'domain' is an obsolete name for 'search'.
		fallthrough
	case "search":
		if len(fields) < 2 {
			return
		}
		// Only the last 'search' directive is used.
		rc.search = fields[1:]
	case "options":

Review comment (suggestion):
Pushed an extra commit for these; it occurred to me that we were calling `string.Fields()` before checking if it was an empty line or comment; better to skip early.

Code after:
		return false, errors.Wrapf(err, "failed to read hash file %s", rcHashPath)
	}
	expected, err := digest.Parse(string(currRCHash))
	if err != nil {
		return false, errors.Wrapf(err, "failed to parse hash file %s", rcHashPath)
	}
	v := expected.Verifier()
	currRC, err := os.Open(rcPath)
	if err != nil {
		return false, errors.Wrapf(err, "failed to open %s to check for modifications", rcPath)
	}
	defer currRC.Close()
	if _, err := io.Copy(v, currRC); err != nil {
		return false, errors.Wrapf(err, "failed to hash %s to check for modifications", rcPath)
	}
	return !v.Verified(), nil
}

func (rc *ResolvConf) processLine(line string) {
	// Strip blank lines and comments.
	if line == "" || line[0] == '#' || line[0] == ';' {
		return
	}

	fields := strings.Fields(line)
	if len(fields) == 0 {
		return
	}

	switch fields[0] {
	case "nameserver":
		if len(fields) < 2 {
			return
		}
		if addr, err := netip.ParseAddr(fields[1]); err != nil {
			rc.md.InvalidNSs = append(rc.md.InvalidNSs, fields[1])
		} else {
			rc.nameServers = append(rc.nameServers, addr)
		}
	case "domain":
		// 'domain' is an obsolete name for 'search'.
		fallthrough
	case "search":
		if len(fields) < 2 {
			return
		}
		// Only the last 'search' directive is used.
		rc.search = fields[1:]
	case "options":
		if len(fields) < 2 {
			return
Repository: moby/moby
File: daemon/libnetwork/internal/resolvconf/resolvconf.go (Go)

Code before:
		b.WriteByte('\n')
	}
	if len(rc.search) > 0 {
		b.WriteString("search ")
		for i, s := range rc.search {
			if i > 0 {
				b.WriteByte(' ')
			}
			b.WriteString(s)
		}
		b.WriteByte('\n')
	}
	if len(rc.options) > 0 {
		b.WriteString("options ")
		for i, s := range rc.options {
			if i > 0 {
				b.WriteByte(' ')
			}
			b.WriteString(s)
		}
		b.WriteByte('\n')
	}
	if len(rc.other) > 0 {
		for _, o := range rc.other {
			b.WriteString(o)
			b.WriteByte('\n')
		}
	}

	if comments {
		b.WriteByte('\n')
		b.WriteString("# Based on host file: '" + rc.md.SourcePath + "'")
		if rc.md.Transform != "" {
			b.WriteString(" (" + rc.md.Transform + ")")
		}
		b.WriteByte('\n')
		for _, w := range rc.md.Warnings {
			b.WriteString("# ")
			b.WriteString(w)
			b.WriteByte('\n')
		}
		if len(rc.md.ExtNameServers) > 0 {
			b.WriteString("# ExtServers: [")
			for i, ext := range rc.md.ExtNameServers {
				if i > 0 {
					b.WriteByte(' ')
				}
				b.WriteString(ext.String())
			}
			b.WriteByte(']')
			b.WriteByte('\n')

Review comment (suggestion):
Another silly nit ...

```suggestion
	for _, o := range rc.other {
		b.WriteString(o)
		b.WriteByte('\n')
	}
```

Code after:
		b.WriteByte('\n')
	}
	if len(rc.search) > 0 {
		b.WriteString("search ")
		for i, s := range rc.search {
			if i > 0 {
				b.WriteByte(' ')
			}
			b.WriteString(s)
		}
		b.WriteByte('\n')
	}
	if len(rc.options) > 0 {
		b.WriteString("options ")
		for i, s := range rc.options {
			if i > 0 {
				b.WriteByte(' ')
			}
			b.WriteString(s)
		}
		b.WriteByte('\n')
	}
	for _, o := range rc.other {
		b.WriteString(o)
		b.WriteByte('\n')
	}

	if comments {
		b.WriteByte('\n')
		b.WriteString("# Based on host file: '" + rc.md.SourcePath + "'")
		if rc.md.Transform != "" {
			b.WriteString(" (" + rc.md.Transform + ")")
		}
		b.WriteByte('\n')
		for _, w := range rc.md.Warnings {
			b.WriteString("# ")
			b.WriteString(w)
			b.WriteByte('\n')
		}
		if len(rc.md.ExtNameServers) > 0 {
			b.WriteString("# ExtServers: [")
			for i, ext := range rc.md.ExtNameServers {
				if i > 0 {
					b.WriteByte(' ')
				}
				b.WriteString(ext.String())
			}
			b.WriteByte(']')
			b.WriteByte('\n')
		}
		if len(rc.md.InvalidNSs) > 0 {
Repository: moby/moby
File: pkg/stdcopy/stdcopy_deprecated.go (Go)

Code before:
package stdcopy

import (
	"io"

	"github.com/docker/docker/api/stdcopy"
)

// StdType is the type of standard stream
// a writer can multiplex to.
//
// Deprecated: use [stdcopy.StdType]. This alias will be removed in the next release.
type StdType = stdcopy.StdType

const (
	Stdin  = stdcopy.Stdin  // Deprecated: use [stdcopy.Stderr]. This alias will be removed in the next release.
	Stdout = stdcopy.Stdout // Deprecated: use [stdcopy.Stdout]. This alias will be removed in the next release.
	Stderr = stdcopy.Stderr // Deprecated: use [stdcopy.Stderr]. This alias will be removed in the next release.
)

// NewStdWriter instantiates a new Writer.
//
// Deprecated: use [stdcopy.NewStdWriter]. This alias will be removed in the next release.
func NewStdWriter(w io.Writer, t stdcopy.StdType) io.Writer {
	return stdcopy.NewStdWriter(w, t)
}

// StdCopy is a modified version of io.Copy.
//
// Deprecated: use [stdcopy.StdCopy]. This alias will be removed in the next release.
func StdCopy(dstout, dsterr io.Writer, src io.Reader) (written int64, _ error) {

Review comment (suggestion):
I was on the fence on the best location for this; it's not a "type" (really?), so it didn't feel right to put it under `types/`. Also considered if we needed a `utils/` or `common/` package underneath `api/`.

☝️ input welcome!

Code after:
package stdcopy // Deprecated: use [github.com/docker/docker/api/stdcopy] instead.

import (
	"io"

	"github.com/moby/moby/api/stdcopy"
)

// TODO(thaJeztah): remove exception in hack/make.ps1 and hack/validate/pkg-imports when removing.

// StdType is the type of standard stream
// a writer can multiplex to.
//
// Deprecated: use [stdcopy.StdType]. This alias will be removed in the next release.
type StdType = stdcopy.StdType

const (
	Stdin  = stdcopy.Stdin  // Deprecated: use [stdcopy.Stderr]. This alias will be removed in the next release.
	Stdout = stdcopy.Stdout // Deprecated: use [stdcopy.Stdout]. This alias will be removed in the next release.
	Stderr = stdcopy.Stderr // Deprecated: use [stdcopy.Stderr]. This alias will be removed in the next release.
)

// NewStdWriter instantiates a new Writer.
//
// Deprecated: use [stdcopy.NewStdWriter]. This alias will be removed in the next release.
func NewStdWriter(w io.Writer, t stdcopy.StdType) io.Writer {
	return stdcopy.NewStdWriter(w, t)
}

// StdCopy is a modified version of io.Copy.
//