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

Code before:
		return nil, fmt.Errorf("RWLayer is unexpectedly nil for container %s", ctr.ID)
	}
	snapshotter := i.client.SnapshotService(ctr.Driver)
	info, err := snapshotter.Stat(ctx, ctr.ID)
	if err != nil {
		return nil, err
	}

	id := stringid.GenerateRandomID()
	parentViewKey := ctr.ID + "-parent-view-" + id
	imageMounts, _ := snapshotter.View(ctx, parentViewKey, info.Parent)

	defer func() {
		if err := snapshotter.Remove(ctx, parentViewKey); err != nil {
			log.G(ctx).WithError(err).Warn("error removing the parent view snapshot")
		}
	}()

	containerRoot, err := rwl.Mount(ctr.GetMountLabel())
	if err != nil {
		return nil, err
	}
	defer func() {
		err := rwl.Unmount()
		if err != nil {
			log.G(ctx).WithError(err).WithField("container", ctr.ID).Warn("Failed to unmount container RWLayer after export")
		}
	}()
	var changes []archive.Change
	err = mount.WithReadonlyTempMount(ctx, imageMounts, func(imageRoot string) error {
		changes, err = archive.ChangesDirs(containerRoot, imageRoot)
		return err
	})
	return changes, err
}


Review comment (bug):
```suggestion
			log.G(ctx).WithFields(log.Fields{"errror": err, "container": ctr.ID}).Warn("Failed to unmount container RWLayer after export")
```

Code after:
		return nil, fmt.Errorf("RWLayer is unexpectedly nil for container %s", ctr.ID)
	}
	snapshotter := i.client.SnapshotService(ctr.Driver)
	info, err := snapshotter.Stat(ctx, ctr.ID)
	if err != nil {
		return nil, err
	}

	id := stringid.GenerateRandomID()
	parentViewKey := ctr.ID + "-parent-view-" + id
	imageMounts, _ := snapshotter.View(ctx, parentViewKey, info.Parent)

	defer func() {
		if err := snapshotter.Remove(ctx, parentViewKey); err != nil {
			log.G(ctx).WithError(err).Warn("error removing the parent view snapshot")
		}
	}()

	containerRoot, err := rwl.Mount(ctr.GetMountLabel())
	if err != nil {
		return nil, err
	}
	defer func() {
		if err := rwl.Unmount(); err != nil {
			log.G(ctx).WithFields(log.Fields{"error": err, "container": ctr.ID}).Warn("Failed to unmount container RWLayer after export")
		}
	}()
	var changes []archive.Change
	err = mount.WithReadonlyTempMount(ctx, imageMounts, func(imageRoot string) error {
		changes, err = archive.ChangesDirs(containerRoot, imageRoot)
		return err
	})
	return changes, err
}
Repository: moby/moby
File: daemon/cluster/executor/container/container.go (Go)

Code before:
func (c *containerConfig) setTask(t *api.Task, node *api.NodeDescription) error {
	if t.Spec.GetContainer() == nil && t.Spec.GetAttachment() == nil {
		return exec.ErrRuntimeUnsupported
	}

	container := t.Spec.GetContainer()
	if container != nil {
		if container.Image == "" {
			return ErrImageRequired
		}

		if err := validateMounts(container.Mounts); err != nil {
			return err
		}
	}

	// index the networks by name
	c.networksAttachments = make(map[string]*api.NetworkAttachment, len(t.Networks))
	for _, attachment := range t.Networks {
		c.networksAttachments[attachment.Network.Spec.Annotations.Name] = attachment
	}

	c.task = t

	if t.Spec.GetContainer() != nil {
		preparedSpec, err := template.ExpandContainerSpec(node, t)
		if err != nil {
			return err
		}
		c.task.Spec.Runtime = &api.TaskSpec_Container{
			Container: preparedSpec,
		}
	}

	return nil
}

func (c *containerConfig) networkAttachmentContainerID() string {
	attachment := c.task.Spec.GetAttachment()
	if attachment == nil {
		return ""
	}

	return attachment.ContainerID
}

func (c *containerConfig) taskID() string {
	return c.task.ID
}

func (c *containerConfig) spec() *api.ContainerSpec {

Review comment (suggestion):
If one of you has thoughts in this, let me know as well; I left it as a TODO here, as it was mostly something I was wondering if it would (should've been) checked on here, but I left it at that (for now)

Code after:
	if t.Spec.GetContainer() == nil && t.Spec.GetAttachment() == nil {
		return exec.ErrRuntimeUnsupported
	}

	container := t.Spec.GetContainer()
	if container != nil {
		if container.Image == "" {
			return ErrImageRequired
		}

		if err := validateMounts(container.Mounts); err != nil {
			return err
		}
	}

	// index the networks by name
	c.networks = make(map[string]*api.Network, len(t.Networks))
	for _, attachment := range t.Networks {
		// It looks like using a map is only for convenience, but not used
		// for validation, nor for looking up the network by name. The name
		// is part of the Network's properties (Network.Spec.Annotations.Name),
		// and effectively only used for debugging; we should consider to
		// change it to a slice.
		//
		// TODO(thaJeztah): should this check for empty and duplicate names?
		c.networks[attachment.Network.Spec.Annotations.Name] = attachment.Network
	}

	c.task = t

	if t.Spec.GetContainer() != nil {
		preparedSpec, err := template.ExpandContainerSpec(node, t)
		if err != nil {
			return err
		}
		c.task.Spec.Runtime = &api.TaskSpec_Container{
			Container: preparedSpec,
		}
	}

	return nil
}

func (c *containerConfig) networkAttachmentContainerID() string {
	attachment := c.task.Spec.GetAttachment()
	if attachment == nil {
		return ""
	}

	return attachment.ContainerID
}
Repository: moby/moby
File: daemon/container.go (Go)

Code before:
	return warnings, err
}

func validateContainerConfig(config *containertypes.Config) error {
	if config == nil {
		return nil
	}
	if err := translateWorkingDir(config); err != nil {
		return err
	}
	if len(config.StopSignal) > 0 {
		if _, err := signal.ParseSignal(config.StopSignal); err != nil {
			return err
		}
	}
	// Validate if Env contains empty variable or not (e.g., ``, `=foo`)
	for _, env := range config.Env {
		if _, err := opts.ValidateEnv(env); err != nil {
			return err
		}
	}
	return validateHealthCheck(config.Healthcheck)
}

func validateHostConfig(hostConfig *containertypes.HostConfig) (warnings []string, _ error) {
	if hostConfig == nil {
		return nil, nil
	}

	if hostConfig.AutoRemove && !hostConfig.RestartPolicy.IsNone() {
		return warnings, errors.Errorf("can't create 'AutoRemove' container with restart policy")
	}
	// Validate mounts; check if host directories still exist
	parser := volumemounts.NewParser()
	for _, c := range hostConfig.Mounts {
		cfg := c
		if err := parser.ValidateMountConfig(&cfg); err != nil {
			return warnings, err
		}
	}
	for _, extraHost := range hostConfig.ExtraHosts {
		if _, err := opts.ValidateExtraHost(extraHost); err != nil {
			return warnings, err
		}
	}
	if err := validatePortBindings(hostConfig.PortBindings); err != nil {
		return warnings, err
	}
	if err := containertypes.ValidateRestartPolicy(hostConfig.RestartPolicy); err != nil {
		return warnings, err
	}

Review comment (suggestion):
Looks like we don't generate any warnings in `validateHostConfig` for now,  the returned `warnings` would be `nil`

Code after:

	return warnings, err
}

func validateContainerConfig(config *containertypes.Config) error {
	if config == nil {
		return nil
	}
	if err := translateWorkingDir(config); err != nil {
		return err
	}
	if len(config.StopSignal) > 0 {
		if _, err := signal.ParseSignal(config.StopSignal); err != nil {
			return err
		}
	}
	// Validate if Env contains empty variable or not (e.g., ``, `=foo`)
	for _, env := range config.Env {
		if _, err := opts.ValidateEnv(env); err != nil {
			return err
		}
	}
	return validateHealthCheck(config.Healthcheck)
}

func validateHostConfig(hostConfig *containertypes.HostConfig) (warnings []string, _ error) {
	if hostConfig == nil {
		return nil, nil
	}

	if hostConfig.AutoRemove && !hostConfig.RestartPolicy.IsNone() {
		return warnings, errors.Errorf("can't create 'AutoRemove' container with restart policy")
	}
	// Validate mounts; check if host directories still exist
	parser := volumemounts.NewParser()
	for _, c := range hostConfig.Mounts {
		cfg := c

		if cfg.Type == mount.TypeImage {
			warnings = append(warnings, "Image mount is an experimental feature")
		}

		if err := parser.ValidateMountConfig(&cfg); err != nil {
			return warnings, err
		}
	}
	for _, extraHost := range hostConfig.ExtraHosts {
		if _, err := opts.ValidateExtraHost(extraHost); err != nil {
			return warnings, err
		}
	}
Repository: moby/moby
File: daemon/list.go (Go)

Code before:
		case stopIteration:
			break
		default:
			filter.idx++
			workers <- struct{}{} // acquire worker slot
			wg.Add(1)

			// worker function
			go func(c *container.Snapshot) {
				defer func() {
					wg.Done()
					<-workers // release worker slot
				}()

				// 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 {
					sizeRw, sizeRootFs, err := daemon.imageService.GetContainerLayerSize(ctx, newC.ID)
					if err != nil {
						return
					}
					newC.SizeRw = sizeRw
					newC.SizeRootFs = sizeRootFs
				}
				results <- newC
			}(currentContainer)
		}
	}
	go func() {
		wg.Wait()
		close(results)
	}()
	for c := range results {
		containers = append(containers, c)
	}
	return containers, nil
}

func (daemon *Daemon) filterByNameIDMatches(view *container.View, filter *listContext) ([]container.Snapshot, error) {
	idSearch := false
	names := filter.filters.Get("name")
	ids := filter.filters.Get("id")
	if len(names)+len(ids) == 0 {
		// if name or ID filters are not in use, return to
		// standard behavior of walking the entire container
		// list from the daemon's in-memory store
		all, err := view.All()
		if err != nil {

Review comment (bug):
It looks like in the old code we would terminate, and return an error here.

Also (but this was the case before); given that we would terminate and return, this would be a case where we can directly assign to the struct fields if we want the code to be slightly briefer;

```go
var err error
newC.SizeRw, newC.SizeRootFs, err = daemon.imageService.GetContainerLayerSize(ctx, newC.ID)
``` 

For the "return on error" case, I'm wondering if we should give https://pkg.go.dev/golang.org/x/sync/errgroup a go; that would also allow us to properly handle the context (if we use `errgroup.WithContext()`).

Code after:
	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
					}
				}

				// insert the result at the given index (so the output is in the
				// same order as containerList above).
				resultsMut.Lock()
				results[idx] = newC
				resultsMut.Unlock()

				return nil
			})
		}
		if stop {
			break
		}
	}

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

	return results[:filter.idx], nil
}

func (daemon *Daemon) filterByNameIDMatches(view *container.View, filter *listContext) ([]container.Snapshot, error) {
	idSearch := false
Repository: moby/moby
File: integration/container/list_test.go (Go)

Code before:
package container

import (
	"fmt"
	"math/rand"
	"testing"

	containertypes "github.com/docker/docker/api/types/container"
	"github.com/docker/docker/api/types/filters"
	"github.com/docker/docker/client"
	"github.com/docker/docker/integration/internal/container"
	"github.com/docker/docker/testutil/request"
	"gotest.tools/v3/assert"
	is "gotest.tools/v3/assert/cmp"
)

func containerListContainsName(containerList []containertypes.Summary, name string) bool {
	for _, ctr := range containerList {
		if ctr.ID == name {
			return true
		}
	}
	return false
}

func TestList(t *testing.T) {
	ctx := setupTest(t)
	apiClient := request.NewAPIClient(t)

	// start a random number of containers (between 0->64)
	num := rand.Intn(64)
	containers := make([]string, num)
	for i := range num {
		id := container.Create(ctx, t, apiClient)
		defer container.Remove(ctx, t, apiClient, id, containertypes.RemoveOptions{Force: true})
		containers[i] = id
	}

	// list them and verify correctness
	containerList, err := apiClient.ContainerList(ctx, containertypes.ListOptions{All: true})
	assert.NilError(t, err)
	assert.Assert(t, is.Len(containerList, num))
	for i := range num {
		assert.Assert(t, containerListContainsName(containerList, containers[i]))

Review comment (suggestion):
I just noticed that we have both a `list_test.go` and  a `ps_test.go`, both with tests for listing; we should probably merge them; https://github.com/moby/moby/blob/126d4bc65b37353121250a93fea896b3a9364a0d/integration/container/ps_test.go#L14

Code after:
package container

import (
	"fmt"
	"testing"

	containertypes "github.com/docker/docker/api/types/container"
	"github.com/docker/docker/api/types/filters"
	"github.com/docker/docker/client"
	"github.com/docker/docker/integration/internal/container"
	"github.com/docker/docker/testutil"
	"github.com/docker/docker/testutil/request"
	"gotest.tools/v3/assert"
	is "gotest.tools/v3/assert/cmp"
	"gotest.tools/v3/skip"
)

func TestContainerList(t *testing.T) {
	ctx := setupTest(t)
	apiClient := request.NewAPIClient(t)

	// remove all existing containers
	container.RemoveAll(ctx, t, apiClient)

	// create the containers
	num := 64
	containers := make([]string, num)
	for i := range num {
		id := container.Create(ctx, t, apiClient)
		defer container.Remove(ctx, t, apiClient, id, containertypes.RemoveOptions{Force: true})
		containers[i] = id
	}

	// list them and verify correctness
	containerList, err := apiClient.ContainerList(ctx, containertypes.ListOptions{All: true})
	assert.NilError(t, err)
	assert.Assert(t, is.Len(containerList, num))
	for i := range num {
		// container list should be ordered in descending creation order
		assert.Assert(t, is.Equal(containerList[i].ID, containers[num-1-i]))
	}
}

func TestContainerList_Annotations(t *testing.T) {
Repository: moby/moby
File: daemon/list_test.go (Go)

Code before:
}

func TestLimitFilter(t *testing.T) {
	db, err := container.NewViewDB()
	assert.NilError(t, err)
	d := &Daemon{
		containersReplica: db,
	}

	// start a number of containers
	num := rand.Intn(64)
	containers := make([]*container.Container, num)
	for i := range num {
		name := fmt.Sprintf("cont-%d", i)
		containers[i] = setupContainerWithName(t, name, d)
	}

	// list them with the limit option and verify correctness; note that the limit
	// option only limits the size of the list, but there are no guarantees as to
	// which containers are in that list (e.g., if limit is 10, it does not mean
	// the list will have the first 10 containers we created; it will have any 10
	// of the containers we created).
	limit := rand.Intn(64)
	containerList, err := d.Containers(context.Background(), &containertypes.ListOptions{Limit: limit})
	assert.NilError(t, err)
	expectedListLen := int(math.Min(float64(num), float64(limit)))
	assert.Assert(t, is.Len(containerList, expectedListLen))
}


Review comment (suggestion):
Can use the builtin `min` (and `max`) now; no need to use the math package 😄 

```suggestion
	expectedListLen := min(num, limit)
```

Code after:
		containersReplica: db,
	}

	// start containers
	num := 32
	for i := range num {
		name := fmt.Sprintf("cont-%d", i)
		setupContainerWithName(t, name, d)
	}

	containers, err := db.Snapshot().All()
	assert.NilError(t, err)
	assert.Assert(t, is.Len(containers, num))

	tests := []struct {
		limit int
		doc   string
	}{
		{limit: 0, doc: "no limit"},
		{limit: -1, doc: "negative limit doesn't limit"},
		{limit: 1, doc: "limit 1 container"},
		{limit: 20, doc: "limit less than num containers"},
		{limit: 32, doc: "limit equal num containers"},
		{limit: 40, doc: "limit greater than num containers"},
	}

	for _, tc := range tests {
		t.Run(tc.doc, func(t *testing.T) {
			containerList, err := d.Containers(context.Background(), &containertypes.ListOptions{Limit: tc.limit})
			assert.NilError(t, err)
			expectedListLen := num
			if tc.limit > 0 {
				expectedListLen = min(num, tc.limit)
			}
			assert.Assert(t, is.Len(containerList, expectedListLen))
		})
	}
}
Repository: moby/moby
File: daemon/list.go (Go)

Code before:
	if numWorkers < 1 {
		numWorkers = 1
	}

	resultsMut := sync.Mutex{}
	results := make([]*containertypes.Summary, 0, min(len(containerList), filter.Limit))
	g, ctx := errgroup.WithContext(ctx)
	g.SetLimit(numWorkers)

	for _, ctr := range containerList {
		switch includeContainerInList(&ctr, filter) {
		case excludeContainer:
			continue
		case stopIteration:
			break
		default:
			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 {
						return err
					}
				}
				resultsMut.Lock()
				results = append(results, newC)
				resultsMut.Unlock()
				return nil
			})
		}
	}

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

	return results, nil
}

func (daemon *Daemon) filterByNameIDMatches(view *container.View, filter *listContext) ([]container.Snapshot, error) {
	idSearch := false
	names := filter.filters.Get("name")
	ids := filter.filters.Get("id")
	if len(names)+len(ids) == 0 {

Review comment (suggestion):
```suggestion
			idx := idx
			filter.idx++
```

Code after:
	// 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
					}
				}

				// insert the result at the given index (so the output is in the
				// same order as containerList above).
				resultsMut.Lock()
				results[idx] = newC
				resultsMut.Unlock()

				return nil
			})
		}
		if stop {
Repository: moby/moby
File: daemon/list.go (Go)

Code before:
			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 {
						return err
					}
				}
				resultsMut.Lock()
				results = append(results, newC)
				resultsMut.Unlock()
				return nil
			})
		}
	}

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

	return results, nil
}

func (daemon *Daemon) filterByNameIDMatches(view *container.View, filter *listContext) ([]container.Snapshot, error) {
	idSearch := false
	names := filter.filters.Get("name")
	ids := filter.filters.Get("id")
	if len(names)+len(ids) == 0 {
		// if name or ID filters are not in use, return to
		// standard behavior of walking the entire container
		// list from the daemon's in-memory store
		all, err := view.All()
		if err != nil {
			return nil, err
		}
		sort.Sort(byCreatedDescending(all))
		return all, nil
	}

	// idSearch will determine if we limit name matching to the IDs
	// matched from any IDs which were specified as filters
	if len(ids) > 0 {
		idSearch = true
	}

	matches := make(map[string]bool)

Review comment (suggestion):
```suggestion
	return results[:filter.idx+1], nil
```

Code after:
		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
					}
				}

				// insert the result at the given index (so the output is in the
				// same order as containerList above).
				resultsMut.Lock()
				results[idx] = newC
				resultsMut.Unlock()

				return nil
			})
		}
		if stop {
			break
		}
	}

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

	return results[:filter.idx], nil
}

func (daemon *Daemon) filterByNameIDMatches(view *container.View, filter *listContext) ([]container.Snapshot, error) {
	idSearch := false
	names := filter.filters.Get("name")
	ids := filter.filters.Get("id")
	if len(names)+len(ids) == 0 {
		// if name or ID filters are not in use, return to
		// standard behavior of walking the entire container