ronantakizawa/codereview-bench huggingface dataset:
Repository: docker/compose
File: pkg/e2e/build_test.go (Go)

Code before:
	res = c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/dependencies/compose.yaml", "build", "secondbuild")
	out = res.Combined()
	assert.Check(t, strings.Contains(out, "secondbuild  Built"))
}

func TestBuildSubDependencies(t *testing.T) {
	c := NewParallelCLI(t)

	t.Cleanup(func() {
		c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/sub-dependencies/compose.yaml", "down", "--rmi=local")
	})

	res := c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/sub-dependencies/compose.yaml", "build", "main")
	out := res.Combined()
	assert.Check(t, strings.Contains(out, "main  Built"))

	res = c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/sub-dependencies/compose.yaml", "up", "--build", "main")
	out = res.Combined()
	assert.Check(t, strings.Contains(out, "main  Built"))
}

func TestBuildLongOutputLine(t *testing.T) {
	c := NewParallelCLI(t)

	t.Cleanup(func() {
		c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/long-output-line/compose.yaml", "down", "--mri=local")
	})

	res := c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/long-output-line/compose.yaml", "build", "long-line")
	out := res.Combined()
	assert.Check(t, strings.Contains(out, "long-line  Built"))

	res = c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/long-output-line/compose.yaml", "up", "--build", "long-line")
	out = res.Combined()
	assert.Check(t, strings.Contains(out, "long-line  Built"))
}


Review comment (suggestion):
```suggestion
		c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/long-output-line/compose.yaml", "down", "--rmi=local")
```

Code after:
	res = c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/dependencies/compose.yaml", "build", "secondbuild")
	out = res.Combined()
	assert.Check(t, strings.Contains(out, "secondbuild  Built"))
}

func TestBuildSubDependencies(t *testing.T) {
	c := NewParallelCLI(t)

	t.Cleanup(func() {
		c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/sub-dependencies/compose.yaml", "down", "--rmi=local")
	})

	res := c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/sub-dependencies/compose.yaml", "build", "main")
	out := res.Combined()
	assert.Check(t, strings.Contains(out, "main  Built"))

	res = c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/sub-dependencies/compose.yaml", "up", "--build", "main")
	out = res.Combined()
	assert.Check(t, strings.Contains(out, "main  Built"))
}

func TestBuildLongOutputLine(t *testing.T) {
	c := NewParallelCLI(t)

	t.Cleanup(func() {
		c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/long-output-line/compose.yaml", "down", "--rmi=local")
	})

	res := c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/long-output-line/compose.yaml", "build", "long-line")
	out := res.Combined()
	assert.Check(t, strings.Contains(out, "long-line  Built"))

	res = c.RunDockerComposeCmd(t, "-f", "fixtures/build-test/long-output-line/compose.yaml", "up", "--build", "long-line")
	out = res.Combined()
	assert.Check(t, strings.Contains(out, "long-line  Built"))
}
Repository: docker/compose
File: cmd/compose/config.go (Go)

Code before:
		return err
	}
	for n := range project.Volumes {
		_, _ = fmt.Fprintln(dockerCli.Out(), n)
	}
	return nil
}

func runNetworks(ctx context.Context, dockerCli command.Cli, opts configOptions) error {
	project, err := opts.ToProject(ctx, dockerCli, nil, cli.WithoutEnvironmentResolution)
	if err != nil {
		return err
	}
	for n := range project.Networks {
		_, _ = fmt.Fprintln(dockerCli.Out(), n)
	}
	return nil
}

func runModels(ctx context.Context, dockerCli command.Cli, opts configOptions) error {
	project, err := opts.ToProject(ctx, dockerCli, nil, cli.WithoutEnvironmentResolution)
	if err != nil {
		return err
	}
	for n := range project.Models {
		_, _ = fmt.Fprintln(dockerCli.Out(), n)
	}
	return nil
}

func runHash(ctx context.Context, dockerCli command.Cli, opts configOptions) error {
	var services []string
	if opts.hash != "*" {
		services = append(services, strings.Split(opts.hash, ",")...)
	}
	project, err := opts.ToProject(ctx, dockerCli, nil, cli.WithoutEnvironmentResolution)
	if err != nil {
		return err
	}

	if err := applyPlatforms(project, true); err != nil {
		return err
	}

	if len(services) == 0 {
		services = project.ServiceNames()
	}

	sorted := services
	sort.Slice(sorted, func(i, j int) bool {
		return sorted[i] < sorted[j]

Review comment (suggestion):
this will dump model name used in compose file, that can't be used in any command. I guess dumping `Model` attribute (OCI artifact ID) would make more sense

Code after:
		return err
	}
	for n := range project.Volumes {
		_, _ = fmt.Fprintln(dockerCli.Out(), n)
	}
	return nil
}

func runNetworks(ctx context.Context, dockerCli command.Cli, opts configOptions) error {
	project, err := opts.ToProject(ctx, dockerCli, nil, cli.WithoutEnvironmentResolution)
	if err != nil {
		return err
	}
	for n := range project.Networks {
		_, _ = fmt.Fprintln(dockerCli.Out(), n)
	}
	return nil
}

func runModels(ctx context.Context, dockerCli command.Cli, opts configOptions) error {
	project, err := opts.ToProject(ctx, dockerCli, nil, cli.WithoutEnvironmentResolution)
	if err != nil {
		return err
	}
	for _, model := range project.Models {
		if model.Model != "" {
			_, _ = fmt.Fprintln(dockerCli.Out(), model.Model)
		}
	}
	return nil
}

func runHash(ctx context.Context, dockerCli command.Cli, opts configOptions) error {
	var services []string
	if opts.hash != "*" {
		services = append(services, strings.Split(opts.hash, ",")...)
	}
	project, err := opts.ToProject(ctx, dockerCli, nil, cli.WithoutEnvironmentResolution)
	if err != nil {
		return err
	}

	if err := applyPlatforms(project, true); err != nil {
		return err
	}

	if len(services) == 0 {
		services = project.ServiceNames()
	}

	sorted := services
Repository: docker/compose
File: internal/sync/tar.go (Go)

Code before:
	if err != nil {
		return err
	}

	var pathsToCopy []PathMapping
	var pathsToDelete []string
	for _, p := range paths {
		if _, err := os.Stat(p.HostPath); err != nil && errors.Is(err, fs.ErrNotExist) {
			pathsToDelete = append(pathsToDelete, p.ContainerPath)
		} else {
			pathsToCopy = append(pathsToCopy, *p)
		}
	}

	var deleteCmd []string
	if len(pathsToDelete) != 0 {
		deleteCmd = append([]string{"rm", "-rf"}, pathsToDelete...)
	}
	var eg multierror.Group
	for i := range containers {
		containerID := containers[i].ID
		tarReader := tarArchive(pathsToCopy)

		eg.Go(func() error {
			if len(deleteCmd) != 0 {
				if err := t.client.Exec(ctx, containerID, deleteCmd, nil); err != nil {
					return fmt.Errorf("deleting paths in %s: %w", containerID, err)
				}
			}

			if err := t.client.Untar(ctx, containerID, tarReader); err != nil {
				return fmt.Errorf("copying files to %s: %w", containerID, err)
			}
			return nil
		})
	}
	return eg.Wait().ErrorOrNil()
}

type ArchiveBuilder struct {
	tw *tar.Writer
	// A shared I/O buffer to help with file copying.
	copyBuf *bytes.Buffer
}

func NewArchiveBuilder(writer io.Writer) *ArchiveBuilder {
	tw := tar.NewWriter(writer)
	return &ArchiveBuilder{
		tw:      tw,
		copyBuf: &bytes.Buffer{},
	}

Review comment (suggestion):
For this one, I used an `errgroup` so that we have the option to limit concurrency, but we could use a regular waitgroup as well

Code after:
	if err != nil {
		return err
	}

	var pathsToCopy []PathMapping
	var pathsToDelete []string
	for _, p := range paths {
		if _, err := os.Stat(p.HostPath); err != nil && errors.Is(err, fs.ErrNotExist) {
			pathsToDelete = append(pathsToDelete, p.ContainerPath)
		} else {
			pathsToCopy = append(pathsToCopy, *p)
		}
	}

	var deleteCmd []string
	if len(pathsToDelete) != 0 {
		deleteCmd = append([]string{"rm", "-rf"}, pathsToDelete...)
	}

	var (
		eg    errgroup.Group
		errMu sync.Mutex
		errs  = make([]error, 0, len(containers)*2) // max 2 errs per container
	)

	eg.SetLimit(16) // arbitrary limit, adjust to taste :D
	for i := range containers {
		containerID := containers[i].ID
		tarReader := tarArchive(pathsToCopy)

		eg.Go(func() error {
			if len(deleteCmd) != 0 {
				if err := t.client.Exec(ctx, containerID, deleteCmd, nil); err != nil {
					errMu.Lock()
					errs = append(errs, fmt.Errorf("deleting paths in %s: %w", containerID, err))
					errMu.Unlock()
				}
			}

			if err := t.client.Untar(ctx, containerID, tarReader); err != nil {
				errMu.Lock()
				errs = append(errs, fmt.Errorf("copying files to %s: %w", containerID, err))
				errMu.Unlock()
			}
			return nil // don't fail-fast; collect all errors
		})
	}

	_ = eg.Wait()
	return errors.Join(errs...)
}
Repository: docker/compose
File: internal/tracing/mux.go (Go)

Code before:
package tracing

import (
	"context"

	"github.com/hashicorp/go-multierror"
	sdktrace "go.opentelemetry.io/otel/sdk/trace"
)

type MuxExporter struct {
	exporters []sdktrace.SpanExporter
}

func (m MuxExporter) ExportSpans(ctx context.Context, spans []sdktrace.ReadOnlySpan) error {
	var eg multierror.Group
	for i := range m.exporters {
		exporter := m.exporters[i]
		eg.Go(func() error {
			return exporter.ExportSpans(ctx, spans)
		})
	}
	return eg.Wait()
}

func (m MuxExporter) Shutdown(ctx context.Context) error {
	var eg multierror.Group
	for i := range m.exporters {
		exporter := m.exporters[i]
		eg.Go(func() error {
			return exporter.Shutdown(ctx)
		})
	}
	return eg.Wait()
}


Review comment (suggestion):
I didn't use a `errgroup` for these, but we could if we want the slightly easier UX. But once we have go1.25 as minimum version, these can be simpified using `WaitGroup.Go`; https://pkg.go.dev/sync#WaitGroup.Go

Code after:
package tracing

import (
	"context"
	"errors"
	"sync"

	sdktrace "go.opentelemetry.io/otel/sdk/trace"
)

type MuxExporter struct {
	exporters []sdktrace.SpanExporter
}

func (m MuxExporter) ExportSpans(ctx context.Context, spans []sdktrace.ReadOnlySpan) error {
	var (
		wg    sync.WaitGroup
		errMu sync.Mutex
		errs  = make([]error, 0, len(m.exporters))
	)

	for _, exporter := range m.exporters {
		wg.Add(1)
		go func() {
			defer wg.Done()
			if err := exporter.ExportSpans(ctx, spans); err != nil {
				errMu.Lock()
				errs = append(errs, err)
				errMu.Unlock()
			}
		}()
	}
	wg.Wait()
	return errors.Join(errs...)
}

func (m MuxExporter) Shutdown(ctx context.Context) error {
	var (
		wg    sync.WaitGroup
		errMu sync.Mutex
		errs  = make([]error, 0, len(m.exporters))
	)

	for _, exporter := range m.exporters {
		wg.Add(1)
		go func() {
			defer wg.Done()
			if err := exporter.Shutdown(ctx); err != nil {
				errMu.Lock()
				errs = append(errs, err)
				errMu.Unlock()
Repository: docker/compose
File: pkg/compose/build_bake.go (Go)

Code before:
	if options.Builder != "" {
		args = append(args, "--builder", options.Builder)
	}
	if options.Quiet {
		args = append(args, "--progress=quiet")
	}

	logrus.Debugf("Executing bake with args: %v", args)

	if s.dryRun {
		return dryRunBake(ctx, cfg), nil
	}
	cmd := exec.CommandContext(ctx, buildx.Path, args...)

	err = s.prepareShellOut(ctx, project.Environment, cmd)
	if err != nil {
		return nil, err
	}
	endpoint, cleanup, err := s.propagateDockerEndpoint()
	if err != nil {
		return nil, err
	}
	cmd.Env = append(cmd.Env, endpoint...)
	defer cleanup()

	fmt.Println(strings.Join(cmd.Env, "\n"))

	cmd.Stdout = s.stdout()
	cmd.Stdin = bytes.NewBuffer(b)
	pipe, err := cmd.StderrPipe()
	if err != nil {
		return nil, err
	}

	var errMessage []string
	reader := bufio.NewReader(pipe)

	err = cmd.Start()
	if err != nil {
		return nil, err
	}
	eg.Go(cmd.Wait)
	for {
		line, readErr := reader.ReadString('\n')
		if readErr != nil {
			if readErr == io.EOF {
				break
			} else {
				return nil, fmt.Errorf("failed to execute bake: %w", readErr)
			}
		}

Review comment (bug):
Not sure we want to print the env variables of the command by default, WDYT?
```suggestion
	logrus.Debugf((strings.Join(cmd.Env, "\n"))
```

Code after:
	if options.Builder != "" {
		args = append(args, "--builder", options.Builder)
	}
	if options.Quiet {
		args = append(args, "--progress=quiet")
	}

	logrus.Debugf("Executing bake with args: %v", args)

	if s.dryRun {
		return dryRunBake(ctx, cfg), nil
	}
	cmd := exec.CommandContext(ctx, buildx.Path, args...)

	err = s.prepareShellOut(ctx, project.Environment, cmd)
	if err != nil {
		return nil, err
	}
	endpoint, cleanup, err := s.propagateDockerEndpoint()
	if err != nil {
		return nil, err
	}
	cmd.Env = append(cmd.Env, endpoint...)
	defer cleanup()

	cmd.Stdout = s.stdout()
	cmd.Stdin = bytes.NewBuffer(b)
	pipe, err := cmd.StderrPipe()
	if err != nil {
		return nil, err
	}

	var errMessage []string
	reader := bufio.NewReader(pipe)

	err = cmd.Start()
	if err != nil {
		return nil, err
	}
	eg.Go(cmd.Wait)
	for {
		line, readErr := reader.ReadString('\n')
		if readErr != nil {
			if readErr == io.EOF {
				break
			} else {
				return nil, fmt.Errorf("failed to execute bake: %w", readErr)
			}
		}
		decoder := json.NewDecoder(strings.NewReader(line))
		var status client.SolveStatus
Repository: docker/compose
File: pkg/compose/publish.go (Go)

Code before:
	if err != nil {
		return err
	}
	if !accept {
		return nil
	}
	err = s.Push(ctx, project, api.PushOptions{IgnoreFailures: true, ImageMandatory: true})
	if err != nil {
		return err
	}

	named, err := reference.ParseDockerRef(repository)
	if err != nil {
		return err
	}

	config := s.dockerCli.ConfigFile()

	resolver := docker.NewResolver(docker.ResolverOptions{
		Hosts: docker.ConfigureDefaultRegistries(
			docker.WithAuthorizer(docker.NewDockerAuthorizer(
				docker.WithAuthCreds(func(host string) (string, string, error) {
					if host == "registry-1.docker.io" {
						host = "https://index.docker.io/v1/"
					}
					auth, err := config.GetAuthConfig(host)
					if err != nil {
						return "", "", err
					}
					if auth.IdentityToken != "" {
						return "", auth.IdentityToken, nil
					}
					return auth.Username, auth.Password, nil
				}),
			)),
		),
	})

	var layers []v1.Descriptor
	extFiles := map[string]string{}
	for _, file := range project.ComposeFiles {
		data, err := processFile(ctx, file, project, extFiles)
		if err != nil {
			return err
		}

		layerDescriptor := ocipush.DescriptorForComposeFile(file, data)
		layers = append(layers, layerDescriptor)
	}

	extLayers, err := processExtends(ctx, project, extFiles)

Review comment (security):
Maybe this should use the internal utility to keep it central? Or is the logic different here? https://github.com/docker/compose/blob/032e0309ee09055d9bc563c63e19b197ed405c1c/internal/registry/registry.go#L30-L38


That one doesn't use `registry-1.docker.io` though; ISTR we had that at some point, but some users depended on `registry-1.docker.io` not to be normalised to `docker.io` (and thus "default auth")


(😭 I need to cry every time I run into these)

Code after:
	accept, err := s.preChecks(project, options)
	if err != nil {
		return err
	}
	if !accept {
		return nil
	}
	err = s.Push(ctx, project, api.PushOptions{IgnoreFailures: true, ImageMandatory: true})
	if err != nil {
		return err
	}

	named, err := reference.ParseDockerRef(repository)
	if err != nil {
		return err
	}

	config := s.dockerCli.ConfigFile()

	resolver := docker.NewResolver(docker.ResolverOptions{
		Hosts: docker.ConfigureDefaultRegistries(
			docker.WithAuthorizer(docker.NewDockerAuthorizer(
				docker.WithAuthCreds(func(host string) (string, string, error) {
					host = registry.GetAuthConfigKey(host)
					auth, err := config.GetAuthConfig(host)
					if err != nil {
						return "", "", err
					}
					if auth.IdentityToken != "" {
						return "", auth.IdentityToken, nil
					}
					return auth.Username, auth.Password, nil
				}),
			)),
		),
	})

	var layers []v1.Descriptor
	extFiles := map[string]string{}
	for _, file := range project.ComposeFiles {
		data, err := processFile(ctx, file, project, extFiles)
		if err != nil {
			return err
		}

		layerDescriptor := ocipush.DescriptorForComposeFile(file, data)
		layers = append(layers, layerDescriptor)
	}

	extLayers, err := processExtends(ctx, project, extFiles)
	if err != nil {
Repository: docker/compose
File: internal/ocipush/push.go (Go)

Code before:
	err := createAndPushManifest(ctx, resolver, named, layerDescriptors, api.OCIVersion1_1)
	var pushErr pusherrors.ErrUnexpectedStatus
	if errors.As(err, &pushErr) && isNonAuthClientError(pushErr.StatusCode) {
		// TODO(milas): show a warning here (won't work with logrus)
		return createAndPushManifest(ctx, resolver, named, layerDescriptors, api.OCIVersion1_0)
	}
	return err
}

func push(ctx context.Context, resolver remotes.Resolver, ref reference.Reference, descriptor v1.Descriptor) error {
	pusher, err := resolver.Pusher(ctx, ref.String())
	if err != nil {
		return err
	}
	push, err := pusher.Push(ctx, descriptor)
	if errdefs.IsAlreadyExists(err) {
		return nil
	}
	if err != nil {
		return err
	}
	_, err = push.Write(descriptor.Data)
	if err != nil {
		return err
	}
	return nil
}

func createAndPushManifest(ctx context.Context, resolver remotes.Resolver, named reference.Named, layers []v1.Descriptor, ociVersion api.OCIVersion) error {
	toPush, err := generateManifest(layers, ociVersion)
	if err != nil {
		return err
	}
	for _, p := range toPush {
		err = push(ctx, resolver, named, p)
		if err != nil {
			return err
		}
	}
	return nil
}

func isNonAuthClientError(statusCode int) bool {
	if statusCode < 400 || statusCode >= 500 {
		// not a client error
		return false
	}
	return !slices.Contains(clientAuthStatusCodes, statusCode)
}

func generateManifest(layers []v1.Descriptor, ociCompat api.OCIVersion) ([]v1.Descriptor, error) {

Review comment (suggestion):
```suggestion
	_, err = push.Write(descriptor.Data)
	return err
```

Code after:
	err := createAndPushManifest(ctx, resolver, named, layerDescriptors, api.OCIVersion1_1)
	var pushErr pusherrors.ErrUnexpectedStatus
	if errors.As(err, &pushErr) && isNonAuthClientError(pushErr.StatusCode) {
		// TODO(milas): show a warning here (won't work with logrus)
		return createAndPushManifest(ctx, resolver, named, layerDescriptors, api.OCIVersion1_0)
	}
	return err
}

func push(ctx context.Context, resolver remotes.Resolver, ref reference.Named, descriptor v1.Descriptor) error {
	fullRef, err := reference.WithDigest(reference.TagNameOnly(ref), descriptor.Digest)
	if err != nil {
		return err
	}

	pusher, err := resolver.Pusher(ctx, fullRef.String())
	if err != nil {
		return err
	}
	push, err := pusher.Push(ctx, descriptor)
	if errdefs.IsAlreadyExists(err) {
		return nil
	}
	if err != nil {
		return err
	}
	defer func() {
		_ = push.Close()
	}()

	_, err = push.Write(descriptor.Data)
	return err
}

func createAndPushManifest(ctx context.Context, resolver remotes.Resolver, named reference.Named, layers []v1.Descriptor, ociVersion api.OCIVersion) error {
	toPush, err := generateManifest(layers, ociVersion)
	if err != nil {
		return err
	}
	for _, p := range toPush {
		err = push(ctx, resolver, named, p)
		if err != nil {
			return err
		}
	}
	return nil
}

func isNonAuthClientError(statusCode int) bool {
	if statusCode < 400 || statusCode >= 500 {
		// not a client error
Repository: docker/compose
File: internal/registry/registry.go (Go)

Code before:
/*
   Copyright 2023 Docker Compose CLI authors

   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
   You may obtain a copy of the License at

       http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
   See the License for the specific language governing permissions and
   limitations under the License.
*/

package registry

import "github.com/docker/docker/registry"

// GetAuthConfigKey special-cases using the full index address of the official
// index as the AuthConfig key, and uses the (host)name[:port] for private indexes.
func GetAuthConfigKey(indexName string) string {
	if indexName == registry.IndexName || indexName == registry.IndexHostname || indexName == registry.DefaultRegistryHost {
		return registry.IndexServer
	}
	return indexName
}


Review comment (suggestion):
OH! Don't import this one; it is removed in the next release / moved internal. It's the package we want to get rid of. 

Code after:
/*
   Copyright 2023 Docker Compose CLI authors

   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
   You may obtain a copy of the License at

       http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
   See the License for the specific language governing permissions and
   limitations under the License.
*/

package registry

const (
	// DefaultNamespace is the default namespace
	DefaultNamespace = "docker.io"
	// DefaultRegistryHost is the hostname for the default (Docker Hub) registry
	// used for pushing and pulling images. This hostname is hard-coded to handle
	// the conversion from image references without registry name (e.g. "ubuntu",
	// or "ubuntu:latest"), as well as references using the "docker.io" domain
	// name, which is used as canonical reference for images on Docker Hub, but
	// does not match the domain-name of Docker Hub's registry.
	DefaultRegistryHost = "registry-1.docker.io"
	// IndexHostname is the index hostname, used for authentication and image search.
	IndexHostname = "index.docker.io"
	// IndexServer is used for user auth and image search
	IndexServer = "https://" + IndexHostname + "/v1/"
	// IndexName is the name of the index
	IndexName = "docker.io"
)

// GetAuthConfigKey special-cases using the full index address of the official
// index as the AuthConfig key, and uses the (host)name[:port] for private indexes.
func GetAuthConfigKey(indexName string) string {
	if indexName == IndexName || indexName == IndexHostname || indexName == DefaultRegistryHost {
		return IndexServer
	}
	return indexName
}