ronantakizawa/codereview-bench huggingface dataset:
Repository: caddyserver/caddy
File: modules/caddyhttp/reverseproxy/upstreams.go (Go)

Code before:
			zap.String("ip", ip.String()))
		upstreams[i] = Upstream{
			Dial: net.JoinHostPort(ip.String(), port),
		}
	}

	// before adding a new one to the cache (as opposed to replacing stale one), make room if cache is full
	if cached.freshness.IsZero() && len(aAaaa) >= 100 {
		for randomKey := range aAaaa {
			delete(aAaaa, randomKey)
			break
		}
	}

	aAaaa[auStr] = aLookup{
		aUpstreams: au,
		freshness:  time.Now(),
		upstreams:  upstreams,
	}

	return allNew(upstreams), nil
}

func (au AUpstreams) String() string { return net.JoinHostPort(au.Name, au.Port) }

func resolveIpVersion(versions *IPVersions) string {
	resolveIpv4 := versions == nil || (versions.IPv4 == nil && versions.IPv6 == nil) || (versions.IPv4 != nil && *versions.IPv4)
	resolveIpv6 := versions == nil || (versions.IPv6 == nil && versions.IPv4 == nil) || (versions.IPv6 != nil && *versions.IPv6)
	switch {
	case resolveIpv4 && !resolveIpv6:
		return "ip4"
	case !resolveIpv4 && resolveIpv6:
		return "ip6"
	default:
		return "ip"
	}
}

type aLookup struct {
	aUpstreams AUpstreams
	freshness  time.Time
	upstreams  []Upstream
}

func (al aLookup) isFresh() bool {
	return time.Since(al.freshness) < time.Duration(al.aUpstreams.Refresh)
}

// MultiUpstreams is a single dynamic upstream source that
// aggregates the results of multiple dynamic upstream sources.
// All configured sources will be queried in order, with their

Review comment (refactor):
I think we should move this function to just below the IPVersions type (higher in this file) since it's tightly related.

Code after:
	au.logger.Debug("refreshing A upstreams",
		zap.String("version", ipVersion),
		zap.String("name", name),
		zap.String("port", port))

	ips, err := au.resolver.LookupIP(r.Context(), ipVersion, name)
	if err != nil {
		return nil, err
	}

	upstreams := make([]Upstream, len(ips))
	for i, ip := range ips {
		au.logger.Debug("discovered A record",
			zap.String("ip", ip.String()))
		upstreams[i] = Upstream{
			Dial: net.JoinHostPort(ip.String(), port),
		}
	}

	// before adding a new one to the cache (as opposed to replacing stale one), make room if cache is full
	if cached.freshness.IsZero() && len(aAaaa) >= 100 {
		for randomKey := range aAaaa {
			delete(aAaaa, randomKey)
			break
		}
	}

	aAaaa[auStr] = aLookup{
		aUpstreams: au,
		freshness:  time.Now(),
		upstreams:  upstreams,
	}

	return allNew(upstreams), nil
}

func (au AUpstreams) String() string { return net.JoinHostPort(au.Name, au.Port) }

type aLookup struct {
	aUpstreams AUpstreams
	freshness  time.Time
	upstreams  []Upstream
}

func (al aLookup) isFresh() bool {
	return time.Since(al.freshness) < time.Duration(al.aUpstreams.Refresh)
}

// MultiUpstreams is a single dynamic upstream source that
// aggregates the results of multiple dynamic upstream sources.
// All configured sources will be queried in order, with their
Repository: caddyserver/caddy
File: modules/caddytls/matchers.go (Go)

Code before:
// Copyright 2015 Matthew Holt and The Caddy 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 caddytls

import (
	"crypto/tls"
	"fmt"
	"net"
	"net/netip"
	"strings"

 	"github.com/caddyserver/certmagic"
 	"go.uber.org/zap"

	"github.com/caddyserver/caddy/v2"
	"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
)

func init() {
	caddy.RegisterModule(MatchServerName{})
	caddy.RegisterModule(MatchRemoteIP{})
	caddy.RegisterModule(MatchLocalIP{})
}

// MatchServerName matches based on SNI. Names in
// this list may use left-most-label wildcards,
// similar to wildcard certificates.
type MatchServerName []string

// CaddyModule returns the Caddy module information.
func (MatchServerName) CaddyModule() caddy.ModuleInfo {
	return caddy.ModuleInfo{
		ID:  "tls.handshake_match.sni",
		New: func() caddy.Module { return new(MatchServerName) },
	}
}

// Match matches hello based on SNI.

Review comment (suggestion):
Oops, I made this suggestion on mobile so it didn't use tabs and has spaces instead. I can't seem to insert a tab character on mobile 🤦‍♂️

Code after:
// Copyright 2015 Matthew Holt and The Caddy 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 caddytls

import (
	"crypto/tls"
	"fmt"
	"net"
	"net/netip"
	"strings"

	"github.com/caddyserver/certmagic"
	"go.uber.org/zap"

	"github.com/caddyserver/caddy/v2"
	"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
)

func init() {
	caddy.RegisterModule(MatchServerName{})
	caddy.RegisterModule(MatchRemoteIP{})
	caddy.RegisterModule(MatchLocalIP{})
}

// MatchServerName matches based on SNI. Names in
// this list may use left-most-label wildcards,
// similar to wildcard certificates.
type MatchServerName []string

// CaddyModule returns the Caddy module information.
func (MatchServerName) CaddyModule() caddy.ModuleInfo {
	return caddy.ModuleInfo{
		ID:  "tls.handshake_match.sni",
		New: func() caddy.Module { return new(MatchServerName) },
	}
}

// Match matches hello based on SNI.
Repository: caddyserver/caddy
File: cmd/commandfactory.go (Go)

Code before:
package caddycmd

import (
	"github.com/spf13/cobra"
)

type RootCommandFactory struct {
	constructor func() *cobra.Command
	options     []func(*cobra.Command)
}

func NewRootCommandFactory(fn func() *cobra.Command) *RootCommandFactory {
	return &RootCommandFactory{
		constructor: fn,
	}
}

func (f *RootCommandFactory) Use(fn func(cmd *cobra.Command)) {
	f.options = append(f.options, fn)
}

func (f *RootCommandFactory) Build() *cobra.Command {
	o := f.constructor()
	for _, v := range f.options {
		v(o)
	}
	return o
}


Review comment (suggestion):
If all this only gets used inside `caddycmd` we could just not export it? Is that fine? Or might other plugins/runtimes want to call this (thinking FrankenPHP, Caddy-Docker-Proxy)

Could use some godoc comments in here to explain the purpose.

Code after:
package caddycmd

import (
	"github.com/spf13/cobra"
)

type rootCommandFactory struct {
	constructor func() *cobra.Command
	options     []func(*cobra.Command)
}

func newRootCommandFactory(fn func() *cobra.Command) *rootCommandFactory {
	return &rootCommandFactory{
		constructor: fn,
	}
}

func (f *rootCommandFactory) Use(fn func(cmd *cobra.Command)) {
	f.options = append(f.options, fn)
}

func (f *rootCommandFactory) Build() *cobra.Command {
	o := f.constructor()
	for _, v := range f.options {
		v(o)
	}
	return o
}
Repository: caddyserver/caddy
File: modules/caddyhttp/fileserver/caddyfile.go (Go)

Code before:
// See UnmarshalCaddyfile for the syntax.
func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) {
	fsrv := new(FileServer)
	err := fsrv.UnmarshalCaddyfile(h.Dispenser)
	if err != nil {
		return fsrv, err
	}
	err = fsrv.FinalizeUnmarshalCaddyfile(h)
	if err != nil {
		return nil, err
	}
	return fsrv, err
}

// UnmarshalCaddyfile parses the file_server directive. It enables
// the static file server and configures it with this syntax:
//
//	file_server [<matcher>] [browse] {
//	    fs            <filesystem>
//	    root          <path>
//	    hide          <files...>
//	    index         <files...>
//	    browse        [<template_file>]
//	    precompressed <formats...>
//	    status        <status>
//	    disable_canonical_uris
//		file_limit	  <limit>
//	}
//
// The FinalizeUnmarshalCaddyfile method should be called after this
// to finalize setup of hidden Caddyfiles.
func (fsrv *FileServer) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
	d.Next() // consume directive name

	args := d.RemainingArgs()
	switch len(args) {
	case 0:
	case 1:
		if args[0] != "browse" {
			return d.ArgErr()
		}
		fsrv.Browse = new(Browse)
	default:
		return d.ArgErr()
	}

	for d.NextBlock(0) {
		switch d.Val() {
		case "fs":
			if !d.NextArg() {
				return d.ArgErr()

Review comment (suggestion):
It's strange, but gofmt tends to format code comments this way.
```suggestion
//	    file_limit      <limit>
```

Code after:
// See UnmarshalCaddyfile for the syntax.
func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) {
	fsrv := new(FileServer)
	err := fsrv.UnmarshalCaddyfile(h.Dispenser)
	if err != nil {
		return fsrv, err
	}
	err = fsrv.FinalizeUnmarshalCaddyfile(h)
	if err != nil {
		return nil, err
	}
	return fsrv, err
}

// UnmarshalCaddyfile parses the file_server directive. It enables
// the static file server and configures it with this syntax:
//
//	file_server [<matcher>] [browse] {
//	    fs            <filesystem>
//	    root          <path>
//	    hide          <files...>
//	    index         <files...>
//	    browse        [<template_file>]
//	    precompressed <formats...>
//	    status        <status>
//	    disable_canonical_uris
//	}
//
// The FinalizeUnmarshalCaddyfile method should be called after this
// to finalize setup of hidden Caddyfiles.
func (fsrv *FileServer) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
	d.Next() // consume directive name

	args := d.RemainingArgs()
	switch len(args) {
	case 0:
	case 1:
		if args[0] != "browse" {
			return d.ArgErr()
		}
		fsrv.Browse = new(Browse)
	default:
		return d.ArgErr()
	}

	for nesting := d.Nesting(); d.NextBlock(nesting); {
		switch d.Val() {
		case "fs":
			if !d.NextArg() {
				return d.ArgErr()
			}
Repository: caddyserver/caddy
File: modules/caddyhttp/fileserver/caddyfile.go (Go)

Code before:
				precompress, ok := inst.(encode.Precompressed)
				if !ok {
					return d.Errf("module %s is not a precompressor; is %T", modID, inst)
				}
				if fsrv.PrecompressedRaw == nil {
					fsrv.PrecompressedRaw = make(caddy.ModuleMap)
				}
				fsrv.PrecompressedRaw[d.Val()] = caddyconfig.JSON(precompress, nil)
				order = append(order, d.Val())
			}
			fsrv.PrecompressedOrder = order

		case "status":
			if !d.NextArg() {
				return d.ArgErr()
			}
			fsrv.StatusCode = caddyhttp.WeakString(d.Val())

		case "disable_canonical_uris":
			if d.NextArg() {
				return d.ArgErr()
			}
			falseBool := false
			fsrv.CanonicalURIs = &falseBool

		case "file_limit":
			fileLimit := d.RemainingArgs()
			if len(fileLimit) != 1 {
				return d.Err("file_limit should have an integer value")
			}
			val, _ := strconv.Atoi(fileLimit[0])
			if fsrv.Browse.FileLimit != 0 {
				return d.Err("file_limit is already enabled")
			}
			fsrv.Browse.FileLimit = val

		case "pass_thru":
			if d.NextArg() {
				return d.ArgErr()
			}
			fsrv.PassThru = true

		case "etag_file_extensions":
			etagFileExtensions := d.RemainingArgs()
			if len(etagFileExtensions) == 0 {
				return d.ArgErr()
			}
			fsrv.EtagFileExtensions = etagFileExtensions

		default:
			return d.Errf("unknown subdirective '%s'", d.Val())

Review comment (suggestion):
This needs to be moved to line 133-ish to be inside of `browse`

Code after:
		case "precompressed":
			var order []string
			for d.NextArg() {
				modID := "http.precompressed." + d.Val()
				mod, err := caddy.GetModule(modID)
				if err != nil {
					return d.Errf("getting module named '%s': %v", modID, err)
				}
				inst := mod.New()
				precompress, ok := inst.(encode.Precompressed)
				if !ok {
					return d.Errf("module %s is not a precompressor; is %T", modID, inst)
				}
				if fsrv.PrecompressedRaw == nil {
					fsrv.PrecompressedRaw = make(caddy.ModuleMap)
				}
				fsrv.PrecompressedRaw[d.Val()] = caddyconfig.JSON(precompress, nil)
				order = append(order, d.Val())
			}
			fsrv.PrecompressedOrder = order

		case "status":
			if !d.NextArg() {
				return d.ArgErr()
			}
			fsrv.StatusCode = caddyhttp.WeakString(d.Val())

		case "disable_canonical_uris":
			if d.NextArg() {
				return d.ArgErr()
			}
			falseBool := false
			fsrv.CanonicalURIs = &falseBool

		case "pass_thru":
			if d.NextArg() {
				return d.ArgErr()
			}
			fsrv.PassThru = true

		case "etag_file_extensions":
			etagFileExtensions := d.RemainingArgs()
			if len(etagFileExtensions) == 0 {
				return d.ArgErr()
			}
			fsrv.EtagFileExtensions = etagFileExtensions

		default:
			return d.Errf("unknown subdirective '%s'", d.Val())
		}
	}
Repository: caddyserver/caddy
File: modules/caddyhttp/reverseproxy/selectionpolicies_test.go (Go)

Code before:
		t.Error("Expected to skip full host.")
	}
}

func TestWeightedRoundRobinPolicyWithZeroWeight(t *testing.T) {
	pool := testPool()
	wrrPolicy := WeightedRoundRobinSelection{
		Weights:     []int{0, 2, 1},
		totalWeight: 3,
	}
	req, _ := http.NewRequest("GET", "/", nil)

	h := wrrPolicy.Select(pool, req, nil)
	if h != pool[1] {
		t.Error("Expected first weighted round robin host to be second host in the pool.")
	}

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[2] {
		t.Error("Expected second weighted round robin host to be third host in the pool.")
	}

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[1] {
		t.Error("Expected third weighted round robin host to be second host in the pool.")
	}

	// mark second host as down
	pool[1].setHealthy(false)
	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[2] {
		t.Error("Expect select next available host.")
	}

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[2] {
		t.Error("Expect select only available host.")
	}

	pool[1].setHealthy(true)

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[1] {
		t.Error("Expect select first host on availability.")
	}

}

func TestLeastConnPolicy(t *testing.T) {
	pool := testPool()
	lcPolicy := LeastConnSelection{}

Review comment (suggestion):
I think this doesn't decisively cover that it would repeat correctly. Maybe it should do like 6 `Select` to make sure four of them are the 2nd and two of them are the 3rd? Cause only selecting 3 times could mean the same as just looping around and selecting the first available again, not being affected by weight.

Code after:
		t.Error("Expected to skip full host.")
	}
}

func TestWeightedRoundRobinPolicyWithZeroWeight(t *testing.T) {
	pool := testPool()
	wrrPolicy := WeightedRoundRobinSelection{
		Weights:     []int{0, 2, 1},
		totalWeight: 3,
	}
	req, _ := http.NewRequest("GET", "/", nil)

	h := wrrPolicy.Select(pool, req, nil)
	if h != pool[1] {
		t.Error("Expected first weighted round robin host to be second host in the pool.")
	}

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[2] {
		t.Error("Expected second weighted round robin host to be third host in the pool.")
	}

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[1] {
		t.Error("Expected third weighted round robin host to be second host in the pool.")
	}

	// mark second host as down
	pool[1].setHealthy(false)
	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[2] {
		t.Error("Expect select next available host.")
	}

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[2] {
		t.Error("Expect select only available host.")
	}
	// mark second host as up
	pool[1].setHealthy(true)

	h = wrrPolicy.Select(pool, req, nil)
	if h != pool[1] {
		t.Error("Expect select first host on availability.")
	}

	// test next select in full cycle
	expected := []*Upstream{pool[1], pool[2], pool[1], pool[1], pool[2], pool[1]}
	for i, want := range expected {
		got := wrrPolicy.Select(pool, req, nil)
		if want != got {
Repository: caddyserver/caddy
File: modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go (Go)

Code before:
				dispenser.DeleteN(len(args) + 1)
				fcgiTransport.CaptureStderr = true
			}
		}
	}

	// reset the dispenser after we're done so that the reverse_proxy
	// unmarshaler can read it from the start
	dispenser.Reset()

	// set up a route list that we'll append to
	routes := caddyhttp.RouteList{}

	// set the list of allowed path segments on which to split
	fcgiTransport.SplitPath = extensions

	// if the index is turned off, we skip the redirect and try_files
	if indexFile != "off" {
		var disableDirRedir bool

		// if tryFiles wasn't overridden, use a reasonable default
		if len(tryFiles) == 0 {
			tryFiles = []string{"{http.request.uri.path}", "{http.request.uri.path}/" + indexFile, indexFile}
		} else {
			longIndexPattern := "{http.request.uri.path}/" + indexFile
			shortIndexPattern := "{path}/" + indexFile
			for _, tf := range tryFiles {
				if tf == longIndexPattern || tf == shortIndexPattern {
					break
				}
			}

			disableDirRedir = true
		}

		if !disableDirRedir {
			// route to redirect to canonical path if index PHP file
			redirMatcherSet := caddy.ModuleMap{
				"file": h.JSON(fileserver.MatchFile{
					TryFiles: []string{"{http.request.uri.path}/" + indexFile},
				}),
				"not": h.JSON(caddyhttp.MatchNot{
					MatcherSetsRaw: []caddy.ModuleMap{
						{
							"path": h.JSON(caddyhttp.MatchPath{"*/"}),
						},
					},
				}),
			}
			redirHandler := caddyhttp.StaticResponse{
				StatusCode: caddyhttp.WeakString(strconv.Itoa(http.StatusPermanentRedirect)),

Review comment (suggestion):
This would never be true, the placeholders shorthands are replaced with regexp before the Caddyfile is parsed. Always assume the longform in here.

Code after:
				dispenser.DeleteN(len(args) + 1)
				fcgiTransport.CaptureStderr = true
			}
		}
	}

	// reset the dispenser after we're done so that the reverse_proxy
	// unmarshaler can read it from the start
	dispenser.Reset()

	// set up a route list that we'll append to
	routes := caddyhttp.RouteList{}

	// set the list of allowed path segments on which to split
	fcgiTransport.SplitPath = extensions

	// if the index is turned off, we skip the redirect and try_files
	if indexFile != "off" {
		dirRedir := false
		dirIndex := "{http.request.uri.path}/" + indexFile

		// if tryFiles wasn't overridden, use a reasonable default
		if len(tryFiles) == 0 {
			tryFiles = []string{"{http.request.uri.path}", dirIndex, indexFile}
			dirRedir = true
		} else {
			for _, tf := range tryFiles {
				if tf == dirIndex {
					dirRedir = true

					break
				}
			}
		}

		if dirRedir {
			// route to redirect to canonical path if index PHP file
			redirMatcherSet := caddy.ModuleMap{
				"file": h.JSON(fileserver.MatchFile{
					TryFiles: []string{dirIndex},
				}),
				"not": h.JSON(caddyhttp.MatchNot{
					MatcherSetsRaw: []caddy.ModuleMap{
						{
							"path": h.JSON(caddyhttp.MatchPath{"*/"}),
						},
					},
				}),
			}
			redirHandler := caddyhttp.StaticResponse{
				StatusCode: caddyhttp.WeakString(strconv.Itoa(http.StatusPermanentRedirect)),
Repository: caddyserver/caddy
File: modules/caddyhttp/headers/caddyfile.go (Go)

Code before:
		}
		err := applyHeaderOp(
			handler.Response.HeaderOps,
			handler.Response,
			field,
			value,
			replacement,
		)
		if err != nil {
			return nil, h.Err(err.Error())
		}
		if len(handler.Response.HeaderOps.Delete) > 0 {
			handler.Response.Deferred = true
		}
	}

	// if not, they should be in a block
	for h.NextBlock(0) {
		field := h.Val()
		if field == "defer" {
			handler.Response.Deferred = true
			continue
		}
		if field == "match" {
			responseMatchers := make(map[string]caddyhttp.ResponseMatcher)
			caddyhttp.ParseNamedResponseMatcher(h.NewFromNextSegment(), responseMatchers)
			matcher := responseMatchers["match"]
			handler.Response.Require = &matcher
			continue
		}
		if hasArgs {
			return nil, h.Err("cannot specify headers in both arguments and block") // because it would be weird
		}

		// sometimes it is habitual for users to suffix a field name with a colon,
		// as if they were writing a curl command or something; see
		// https://caddy.community/t/v2-reverse-proxy-please-add-cors-example-to-the-docs/7349/19
		field = strings.TrimSuffix(field, ":")

		var value string
		var replacement *string
		if h.NextArg() {
			value = h.Val()
		}
		if h.NextArg() {
			arg := h.Val()
			replacement = &arg
		}

		handlerToUse := handler
		if strings.HasPrefix(field, "?") {

Review comment (suggestion):
```suggestion
			err := caddyhttp.ParseNamedResponseMatcher(h.NewFromNextSegment(), responseMatchers)
			if err != nil {
				return nil, err
			}
```

Code after:
		}
		err := applyHeaderOp(
			handler.Response.HeaderOps,
			handler.Response,
			field,
			value,
			replacement,
		)
		if err != nil {
			return nil, h.Err(err.Error())
		}
		if len(handler.Response.HeaderOps.Delete) > 0 {
			handler.Response.Deferred = true
		}
	}

	// if not, they should be in a block
	for h.NextBlock(0) {
		field := h.Val()
		if field == "defer" {
			handler.Response.Deferred = true
			continue
		}
		if field == "match" {
			responseMatchers := make(map[string]caddyhttp.ResponseMatcher)
			err := caddyhttp.ParseNamedResponseMatcher(h.NewFromNextSegment(), responseMatchers)
			if err != nil {
				return nil, err
			}
			matcher := responseMatchers["match"]
			handler.Response.Require = &matcher
			continue
		}
		if hasArgs {
			return nil, h.Err("cannot specify headers in both arguments and block") // because it would be weird
		}

		// sometimes it is habitual for users to suffix a field name with a colon,
		// as if they were writing a curl command or something; see
		// https://caddy.community/t/v2-reverse-proxy-please-add-cors-example-to-the-docs/7349/19
		field = strings.TrimSuffix(field, ":")

		var value string
		var replacement *string
		if h.NextArg() {
			value = h.Val()
		}
		if h.NextArg() {
			arg := h.Val()
			replacement = &arg
		}