ronantakizawa/codereview-bench huggingface dataset:
Repository: syncthing/syncthing
File: cmd/syncthing/support_bundle.go (Go)

Code before:
	"archive/zip"
	"io"
	"io/ioutil"
	"net/http"
	"path/filepath"
	"strconv"
	"strings"
	"time"

	"github.com/syncthing/syncthing/lib/config"
)

// AddBufferFile adds new json to fileByte(name, bytes.Buffer)
func addBufferFile(arrayFile []fileByte, name string, json []byte) []fileByte {
	var singleFile fileByte
	singleFile.name = name
	singleFile.buf.Write(json)
	arrayFile = append(arrayFile, singleFile)
	return arrayFile
}

// GetRedactedConfig redacting some parts of config
func getRedactedConfig(s *apiService) config.Configuration {
	rawConf := s.cfg.RawCopy()
	rawConf.GUI.APIKey = "REDACTED"
	rawConf.GUI.Password = "REDACTED"
	rawConf.GUI.User = "REDACTED"
	return rawConf
}

// GetusageReportingData collects needed data for support personnel
func getusageReportingData(s *apiService, r *http.Request) map[string]interface{} {
	data := make(map[string]interface{})
	folders := make(map[string]interface{})

	version := usageReportVersion
	if val, _ := strconv.Atoi(r.URL.Query().Get("version")); val > 0 {
		version = val
	}
	repData := reportData(s.cfg, s.model, s.connectionsService, version, true)

	data["hashPerf"] = repData["hashPerf"]
	data["sha256Perf"] = repData["sha256Perf"]
	data["memorySize"] = repData["memorySize"]
	data["memoryUsageMiB"] = repData["memoryUsageMiB"]
	data["numCPU"] = repData["numCPU"]
	data["numDevices"] = repData["numDevices"]

	folders["numFolders"] = repData["numFolders"]
	folders["folderMaxFiles"] = repData["folderMaxFiles"]
	folders["folderMaxMiB"] = repData["folderMaxMiB"]

Review comment (security):
Only do this if the User/Password value are set, to preserve the fact that no authentication is set when it isn't.

```
guiCfg := s.cfg.GUI()
if guiCfg.Password != "" {
   guiCfg.Password = "REDACTED"
}
...
```

Code after:
	"archive/zip"
	"io"

	"github.com/syncthing/syncthing/lib/config"
)

// getRedactedConfig redacting some parts of config
func getRedactedConfig(s *apiService) config.Configuration {
	rawConf := s.cfg.RawCopy()
	rawConf.GUI.APIKey = "REDACTED"
	if rawConf.GUI.Password != "" {
		rawConf.GUI.Password = "REDACTED"
	}
	if rawConf.GUI.User != "" {
		rawConf.GUI.User = "REDACTED"
	}
	return rawConf
}

// writeZip writes a zip file containing the given entries
func writeZip(writer io.Writer, files []fileEntry) error {
	zipWriter := zip.NewWriter(writer)
	defer zipWriter.Close()

	for _, file := range files {
		zipFile, err := zipWriter.Create(file.name)
		if err != nil {
			return err
		}

		_, err = zipFile.Write(file.data)
		if err != nil {
			return err
		}
	}

	return zipWriter.Close()
}
Repository: syncthing/syncthing
File: cmd/syncthing/support_bundle.go (Go)

Code before:
	"archive/zip"
	"io"

	"github.com/syncthing/syncthing/lib/config"
)

// GetRedactedConfig redacting some parts of config
func getRedactedConfig(s *apiService) config.Configuration {
	rawConf := s.cfg.RawCopy()
	rawConf.GUI.APIKey = "REDACTED"
	if rawConf.GUI.Password != "" {
		rawConf.GUI.Password = "REDACTED"
	}
	if rawConf.GUI.User != "" {
		rawConf.GUI.User = "REDACTED"
	}
	return rawConf
}

// ZipBuffer creates a zip and adds fileEntry(name, []byte]) array to zip
func zipBuffer(writer io.Writer, files []fileEntry) {
	zipWriter := zip.NewWriter(writer)
	defer func() {
		err := zipWriter.Close()
		if err != nil {
			l.Infoln("Zipwriter not close:", err)
		}
	}()

	// Add files to zip
	for _, file := range files {
		zipFile, err := zipWriter.Create(file.name)
		if err != nil {
			l.Infoln("Zipwriter not create filename:", err)
		}
		_, err = zipFile.Write(file.data)
		if err != nil {
			l.Infoln("Zipwriter not writes:", err)
		}
	}
}


Review comment (suggestion):
This probably would return an error, instead of doing logging itself, then you only have to log once, and return upon receiving an error.

Code after:
	"archive/zip"
	"io"

	"github.com/syncthing/syncthing/lib/config"
)

// getRedactedConfig redacting some parts of config
func getRedactedConfig(s *apiService) config.Configuration {
	rawConf := s.cfg.RawCopy()
	rawConf.GUI.APIKey = "REDACTED"
	if rawConf.GUI.Password != "" {
		rawConf.GUI.Password = "REDACTED"
	}
	if rawConf.GUI.User != "" {
		rawConf.GUI.User = "REDACTED"
	}
	return rawConf
}

// writeZip writes a zip file containing the given entries
func writeZip(writer io.Writer, files []fileEntry) error {
	zipWriter := zip.NewWriter(writer)
	defer zipWriter.Close()

	for _, file := range files {
		zipFile, err := zipWriter.Create(file.name)
		if err != nil {
			return err
		}

		_, err = zipFile.Write(file.data)
		if err != nil {
			return err
		}
	}

	return zipWriter.Close()
}
Repository: syncthing/syncthing
File: cmd/syncthing/support_bundle.go (Go)

Code before:
	return rawConf
}

// ZipBuffer creates a zip and adds fileEntry(name, []byte]) array to zip
func zipBuffer(writer io.Writer, files []fileEntry) error {
	zipWriter := zip.NewWriter(writer)
	defer func() {
		err := zipWriter.Close()
		if err != nil {
			l.Infoln("Zipwriter not close:", err)
			return
		}
	}()

	// Add files to zip
	for _, file := range files {
		zipFile, err := zipWriter.Create(file.name)
		if err != nil {
			l.Infoln("Zipwriter not create filename:", err)
		}
		_, err = zipFile.Write(file.data)
		if err != nil {
			return err
		}
	}
	return nil
}


Review comment (suggestion):
replace with `return zipWriter.Close()`, and make the defer simply double close in case of success (which should be noop)

Code after:
// Copyright (C) 2018 The Syncthing Authors.
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

package main

import (
	"archive/zip"
	"io"

	"github.com/syncthing/syncthing/lib/config"
)

// getRedactedConfig redacting some parts of config
func getRedactedConfig(s *apiService) config.Configuration {
	rawConf := s.cfg.RawCopy()
	rawConf.GUI.APIKey = "REDACTED"
	if rawConf.GUI.Password != "" {
		rawConf.GUI.Password = "REDACTED"
	}
	if rawConf.GUI.User != "" {
		rawConf.GUI.User = "REDACTED"
	}
	return rawConf
}

// writeZip writes a zip file containing the given entries
func writeZip(writer io.Writer, files []fileEntry) error {
	zipWriter := zip.NewWriter(writer)
	defer zipWriter.Close()

	for _, file := range files {
		zipFile, err := zipWriter.Create(file.name)
		if err != nil {
			return err
		}

		_, err = zipFile.Write(file.data)
		if err != nil {
			return err
		}
	}

	return zipWriter.Close()
}
Repository: syncthing/syncthing
File: lib/protocol/bep_extensions.go (Go)

Code before:

	// Mask out the ignored local flags before checking IsInvalid() below
	f.LocalFlags &^= ignoreFlags
	other.LocalFlags &^= ignoreFlags

	if f.Name != other.Name || f.Type != other.Type || f.Deleted != other.Deleted || f.IsInvalid() != other.IsInvalid() {
		return false
	}

	if !ignorePerms && !f.NoPermissions && !other.NoPermissions && !PermsEqual(f.Permissions, other.Permissions) {
		return false
	}

	switch f.Type {
	case FileInfoTypeFile:
		return f.Size == other.Size && f.ModTime().Equal(other.ModTime()) && (ignoreBlocks || BlocksEqual(f.Blocks, other.Blocks))
	case FileInfoTypeSymlink:
		return f.SymlinkTarget == other.SymlinkTarget
	case FileInfoTypeDirectory:
		return true
	}

	return false
}

func FromFSFileInfo(fi fs.FileInfo, name string, filesystem fs.Filesystem) FileInfo {
	f := FileInfo{
		Name:        name,
		Type:        FileInfoTypeFile,
		Permissions: uint32(fi.Mode()),
		ModifiedS:   fi.ModTime().Unix(),
		ModifiedNs:  int32(fi.ModTime().Nanosecond()),
		Size:        fi.Size(),
	}
	switch {
	case fi.IsDir():
		f.Type = FileInfoTypeDirectory
	case fi.IsSymlink():
		f.Type = FileInfoTypeSymlink
		target, _ := filesystem.ReadSymlink(name)
		f.SymlinkTarget = target
	}
	return f
}

func PermsEqual(a, b uint32) bool {
	switch runtime.GOOS {
	case "windows":
		// There is only writeable and read only, represented for user, group
		// and other equally. We only compare against user.
		return a&0600 == b&0600

Review comment (refactor):
I haven't reviewed anything yet, just happened to scroll to the bottom and saw this - i don't think we want to add a dependency on `fs` from `protocol`, so probably better to have this helper where it's used... or maybe scanner?

Code after:
	// Mask out the ignored local flags before checking IsInvalid() below
	f.LocalFlags &^= ignoreFlags
	other.LocalFlags &^= ignoreFlags

	if f.Name != other.Name || f.Type != other.Type || f.Deleted != other.Deleted || f.IsInvalid() != other.IsInvalid() {
		return false
	}

	if !ignorePerms && !f.NoPermissions && !other.NoPermissions && !PermsEqual(f.Permissions, other.Permissions) {
		return false
	}

	switch f.Type {
	case FileInfoTypeFile:
		return f.Size == other.Size && f.ModTime().Equal(other.ModTime()) && (ignoreBlocks || BlocksEqual(f.Blocks, other.Blocks))
	case FileInfoTypeSymlink:
		return f.SymlinkTarget == other.SymlinkTarget
	case FileInfoTypeDirectory:
		return true
	}

	return false
}

func PermsEqual(a, b uint32) bool {
	switch runtime.GOOS {
	case "windows":
		// There is only writeable and read only, represented for user, group
		// and other equally. We only compare against user.
		return a&0600 == b&0600
	default:
		// All bits count
		return a&0777 == b&0777
	}
}

// BlocksEqual returns whether two slices of blocks are exactly the same hash
// and index pair wise.
func BlocksEqual(a, b []BlockInfo) bool {
	if len(b) != len(a) {
		return false
	}

	for i, sblk := range a {
		if !bytes.Equal(sblk.Hash, b[i].Hash) {
			return false
		}
	}

	return true
}
Repository: syncthing/syncthing
File: lib/logger/logger.go (Go)

Code before:

// Fatalf logs a formatted line with a FATAL prefix and exits the process with
// exit code 1.
func (l *logger) Fatalf(format string, vals ...interface{}) {
	s := fmt.Sprintf(format, vals...)
	l.mut.Lock()
	defer l.mut.Unlock()
	l.logger.Output(2, "FATAL: "+s)
	l.callHandlers(LevelFatal, s)
	os.Exit(1)
}

// ShouldDebug returns true if the given facility has debugging enabled.
func (l *logger) ShouldDebug(facility string) bool {
	l.mut.Lock()
	res := l.debug[facility]
	l.mut.Unlock()
	return res
}

// SetDebug enabled or disables debugging for the given facility name.
func (l *logger) SetDebug(facility string, enabled bool) {
	l.mut.Lock()
	defer l.mut.Unlock()
	l.debug[facility] = enabled
	if enabled {
		l.SetFlags(DebugFlags)
		return
	}
	for _, enabled := range l.debug {
		if enabled {
			return
		}
	}
	l.SetFlags(DefaultFlags)
}

// FacilityDebugging returns the set of facilities that have debugging
// enabled.
func (l *logger) FacilityDebugging() []string {
	var enabled []string
	l.mut.Lock()
	for facility, isEnabled := range l.debug {
		if isEnabled {
			enabled = append(enabled, facility)
		}
	}
	l.mut.Unlock()
	return enabled
}


Review comment (bug):
l.debug could be map[string]struct{} where entry being there implies its on, entry not being there implies its off. Then enabled or not is just a len() over the map.

Also, I'd want to add shortfile to default log flags, @calmh?

Code after:
}

// Fatalf logs a formatted line with a FATAL prefix and exits the process with
// exit code 1.
func (l *logger) Fatalf(format string, vals ...interface{}) {
	s := fmt.Sprintf(format, vals...)
	l.mut.Lock()
	defer l.mut.Unlock()
	l.logger.Output(2, "FATAL: "+s)
	l.callHandlers(LevelFatal, s)
	os.Exit(1)
}

// ShouldDebug returns true if the given facility has debugging enabled.
func (l *logger) ShouldDebug(facility string) bool {
	l.mut.Lock()
	_, res := l.debug[facility]
	l.mut.Unlock()
	return res
}

// SetDebug enabled or disables debugging for the given facility name.
func (l *logger) SetDebug(facility string, enabled bool) {
	l.mut.Lock()
	defer l.mut.Unlock()
	if _, ok := l.debug[facility]; enabled && !ok {
		l.SetFlags(DebugFlags)
		l.debug[facility] = struct{}{}
	} else if !enabled && ok {
		delete(l.debug, facility)
		if len(l.debug) == 0 {
			l.SetFlags(DefaultFlags)
		}
	}
}

// FacilityDebugging returns the set of facilities that have debugging
// enabled.
func (l *logger) FacilityDebugging() []string {
	enabled := make([]string, 0, len(l.debug))
	l.mut.Lock()
	for facility := range l.debug {
		enabled = append(enabled, facility)
	}
	l.mut.Unlock()
	return enabled
}

// Facilities returns the currently known set of facilities and their
// descriptions.
func (l *logger) Facilities() map[string]string {
Repository: syncthing/syncthing
File: cmd/syncthing/gui.go (Go)

Code before:
	// ends with a path separator
	var searchFile string
	if !strings.HasSuffix(search, pathSeparator) {
		searchFile = filepath.Base(search)
	}

	fs := fs.NewFilesystem(fsType, searchDir)

	subdirectories, _ := fs.DirNames("")

	type match struct {
		path             string
		case_differences int
	}

	matches := make([]match, 0, len(subdirectories))

next_subdir:
	for _, subdirectory := range subdirectories {
		var m match
		for i := 0; i < len(subdirectory) && i < len(searchFile); i += 1 {
			if subdirectory[i] == searchFile[i] {
				continue
			}

			if strings.EqualFold(string(subdirectory[i]), string(searchFile[i])) {
				m.case_differences += 1
				continue
			}

			continue next_subdir
		}

		info, err := fs.Stat(subdirectory)
		if err == nil && info.IsDir() {
			m.path = filepath.Join(searchDir, subdirectory) + pathSeparator
			matches = append(matches, m)
		}
	}

	// sort by number of differences with pattern
	// ties are resolved by alphabetical order
	sort.Slice(matches, func(i, j int) bool {
		if matches[i].case_differences == matches[j].case_differences {
			return strings.ToLower(matches[i].path) < strings.ToLower(matches[j].path)
		}
		return matches[i].case_differences < matches[j].case_differences
	})

	ret := make([]string, 0, len(matches))
	for _, m := range matches {

Review comment (refactor):
This can be funky with utf8 where a character is more than one byte, perhaps you should extract runes using utf8 package, and do unicode.ToLower(rune) etc.

Code after:

func browseFiles(current string, fsType fs.FilesystemType) []string {
	if current == "" {
		filesystem := fs.NewFilesystem(fsType, "")
		if roots, err := filesystem.Roots(); err == nil {
			return roots
		}
		return nil
	}
	search, _ := fs.ExpandTilde(current)
	pathSeparator := string(fs.PathSeparator)

	if strings.HasSuffix(current, pathSeparator) && !strings.HasSuffix(search, pathSeparator) {
		search = search + pathSeparator
	}
	searchDir := filepath.Dir(search)

	// The searchFile should be the last component of search, or empty if it
	// ends with a path separator
	var searchFile string
	if !strings.HasSuffix(search, pathSeparator) {
		searchFile = filepath.Base(search)
	}

	fs := fs.NewFilesystem(fsType, searchDir)

	subdirectories, _ := fs.DirNames(".")

	exactMatches := make([]string, 0, len(subdirectories))
	caseInsMatches := make([]string, 0, len(subdirectories))

	for _, subdirectory := range subdirectories {
		info, err := fs.Stat(subdirectory)
		if err != nil || !info.IsDir() {
			continue
		}

		switch checkPrefixMatch(subdirectory, searchFile) {
		case matchExact:
			exactMatches = append(exactMatches, filepath.Join(searchDir, subdirectory)+pathSeparator)
		case matchCaseIns:
			caseInsMatches = append(caseInsMatches, filepath.Join(searchDir, subdirectory)+pathSeparator)
		}
	}

	// sort to return matches in deterministic order (don't depend on file system order)
	sort.Strings(exactMatches)
	sort.Strings(caseInsMatches)
	return append(exactMatches, caseInsMatches...)
}
Repository: syncthing/syncthing
File: cmd/syncthing/gui_test.go (Go)

Code before:
		returns []string
	}{
		// The direcotory without slash is completed to one with slash.
		{tmpDir, []string{tmpDir + pathSep}},
		// With slash it's completed to its contents.
		// Dirs are given pathSeps.
		// Files are not returned.
		{tmpDir + pathSep, []string{dirPath, mixedCaseDirPath}},
		// Globbing is automatic based on prefix.
		{tmpDir + pathSep + "d", []string{dirPath}},
		{tmpDir + pathSep + "di", []string{dirPath}},
		{tmpDir + pathSep + "dir", []string{dirPath}},
		{tmpDir + pathSep + "f", nil},
		{tmpDir + pathSep + "q", nil},
		// Globbing is case-insensitve
		{tmpDir + pathSep + "mixed", []string{mixedCaseDirPath}},
	}

	for _, tc := range cases {
		ret := browseFiles(tc.current, fs.FilesystemTypeBasic)
		if !equalStrings(ret, tc.returns) {
			t.Errorf("browseFiles(%q) => %q, expected %q", tc.current, ret, tc.returns)
		}
	}

	// test mixed case sorting with fakefs
	fake := fs.NewFilesystem(fs.FilesystemTypeFake, tmpDir)
	fake.Mkdir("aaaa", 0755)
	fake.Mkdir("aaaA", 0755)
	fake.Mkdir("Aaaa", 0755)
	fake.Mkdir("AaaA", 0755)
	ret := browseFiles(tmpDir+pathSep+"aaaA", fs.FilesystemTypeFake)
	equalStrings(ret, []string{
		// exact match first
		"aaaA",
		// remaining matches sorted by number of differences
		"aaaa",
		"Aaaa",
		"AaaA",
	})
}

func equalStrings(a, b []string) bool {
	if len(a) != len(b) {
		return false
	}
	for i := range a {
		if a[i] != b[i] {
			return false
		}
	}

Review comment (suggestion):
This should really be a separate test, and could be on prefixCaseDifferences directly to avoid the filesystem shenanigans

Code after:
		returns []string
	}{
		// The direcotory without slash is completed to one with slash.
		{tmpDir, []string{tmpDir + pathSep}},
		// With slash it's completed to its contents.
		// Dirs are given pathSeps.
		// Files are not returned.
		{tmpDir + pathSep, []string{mixedCaseDirPath, dirPath}},
		// Globbing is automatic based on prefix.
		{tmpDir + pathSep + "d", []string{dirPath}},
		{tmpDir + pathSep + "di", []string{dirPath}},
		{tmpDir + pathSep + "dir", []string{dirPath}},
		{tmpDir + pathSep + "f", nil},
		{tmpDir + pathSep + "q", nil},
		// Globbing is case-insensitve
		{tmpDir + pathSep + "mixed", []string{mixedCaseDirPath}},
	}

	for _, tc := range cases {
		ret := browseFiles(tc.current, fs.FilesystemTypeBasic)
		if !equalStrings(ret, tc.returns) {
			t.Errorf("browseFiles(%q) => %q, expected %q", tc.current, ret, tc.returns)
		}
	}
}

func TestPrefixMatch(t *testing.T) {
	cases := []struct {
		s        string
		prefix   string
		expected int
	}{
		{"aaaA", "aaa", matchExact},
		{"AAAX", "BBB", noMatch},
		{"AAAX", "aAa", matchCaseIns},
		{"äÜX", "äü", matchCaseIns},
	}

	for _, tc := range cases {
		ret := checkPrefixMatch(tc.s, tc.prefix)
		if ret != tc.expected {
			t.Errorf("checkPrefixMatch(%q, %q) => %v, expected %v", tc.s, tc.prefix, ret, tc.expected)
		}
	}
}

func equalStrings(a, b []string) bool {
	if len(a) != len(b) {
		return false
	}
	for i := range a {
Repository: syncthing/syncthing
File: lib/protocol/protocol.go (Go)

Code before:

	case CompressMetadata:
		_, isResponse := msg.(*Response)
		// Compress if it's large enough and not a response message
		return !isResponse && msg.ProtoSize() >= compressionThreshold

	default:
		panic("unknown compression setting")
	}
}

// Close is called when the connection is regularely closed and thus the Close
// BEP message is sent before terminating the actual connection. The error
// argument specifies the reason for closing the connection.
func (c *rawConnection) Close(err error) {
	c.once.Do(func() {
		done := make(chan struct{})
		c.sendClose <- asyncMessage{&Close{err.Error()}, done}
		<-done

		go c.commonClose(err)
	})
}

// close is called if there is an unexpected error during normal operation.
func (c *rawConnection) close(err error) {
	c.once.Do(func() {
		c.commonClose(err)
	})
}

// commonClose is a utility function that must only be called from within
// rawConnection.once.Do (i.e. in Close and close).
func (c *rawConnection) commonClose(err error) {
	l.Debugln("close due to", err)
	close(c.closed)

	c.awaitingMut.Lock()
	for i, ch := range c.awaiting {
		if ch != nil {
			close(ch)
			delete(c.awaiting, i)
		}
	}
	c.awaitingMut.Unlock()

	c.receiver.Closed(c, err)
}

// The pingSender makes sure that we've sent a message within the last
// PingSendInterval. If we already have something sent in the last

Review comment (suggestion):
I'm not super fond of the having both `close()` and `Close()`, the potential for confusion is enormous... I'm slightly confused even now. 😕 <- look, confused emoji

Code after:

	case CompressMetadata:
		_, isResponse := msg.(*Response)
		// Compress if it's large enough and not a response message
		return !isResponse && msg.ProtoSize() >= compressionThreshold

	default:
		panic("unknown compression setting")
	}
}

// Close is called when the connection is regularely closed and thus the Close
// BEP message is sent before terminating the actual connection. The error
// argument specifies the reason for closing the connection.
func (c *rawConnection) Close(err error) {
	c.once.Do(func() {
		done := make(chan struct{})
		c.sendClose <- asyncMessage{&Close{err.Error()}, done}
		<-done

		// No more sends are necessary, therefore closing the underlying
		// connection can happen at the same time as the internal cleanup.
		// And this prevents a potential deadlock due to calling c.receiver.Closed
		go c.commonClose(err)
	})
}

// internalClose is called if there is an unexpected error during normal operation.
func (c *rawConnection) internalClose(err error) {
	c.once.Do(func() {
		c.commonClose(err)
	})
}

// commonClose is a utility function that must only be called from within
// rawConnection.once.Do (i.e. in Close and close).
func (c *rawConnection) commonClose(err error) {
	l.Debugln("close due to", err)
	close(c.closed)

	c.awaitingMut.Lock()
	for i, ch := range c.awaiting {
		if ch != nil {
			close(ch)
			delete(c.awaiting, i)
		}
	}
	c.awaitingMut.Unlock()

	c.receiver.Closed(c, err)
}