ronantakizawa/codereview-bench huggingface dataset:
Repository: syncthing/syncthing
File: lib/osutil/lan.go (Go)

Code before:
// Copyright (C) 2015 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 osutil

import (
	"net"
)

func GetLans() ([]*net.IPNet, error) {
	ifs, err := net.Interfaces()
	if err != nil {
		return nil, err
	}
	upIfs := []net.Interface{}
	for _, currentIf := range ifs {
		if currentIf.Flags&net.FlagUp == net.FlagUp {
			upIfs = append(upIfs, currentIf)
		}
	}
	addrs := []net.Addr{}
	for _, currentUpIf := range upIfs {
		currentAddrs, err := currentUpIf.Addrs()
		if err != nil {
			return nil, err
		}
		for _, currentAddr := range currentAddrs {
			addrs = append(addrs, currentAddr)
		}
	}

	nets := make([]*net.IPNet, 0, len(addrs))

	for _, addr := range addrs {
		net, ok := addr.(*net.IPNet)
		if ok {
			nets = append(nets, net)
		}
	}
	return nets, nil
}


Review comment (suggestion):
```suggestion
	addrs := []net.Addr{}
	for _, currentIf := range ifs {
		if currentIf.Flags&net.FlagUp != net.FlagUp {
			continue
		}
```

Code after:
// Copyright (C) 2015 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 osutil

import (
	"net"
)

func GetLans() ([]*net.IPNet, error) {
	ifs, err := net.Interfaces()
	if err != nil {
		return nil, err
	}
	addrs := []net.Addr{}
	for _, currentIf := range ifs {
		if currentIf.Flags&net.FlagUp != net.FlagUp {
			continue
		}
		currentAddrs, err := currentIf.Addrs()
		if err != nil {
			return nil, err
		}
		addrs = append(addrs, currentAddrs...)
	}

	nets := make([]*net.IPNet, 0, len(addrs))

	for _, addr := range addrs {
		net, ok := addr.(*net.IPNet)
		if ok {
			nets = append(nets, net)
		}
	}
	return nets, nil
}
Repository: syncthing/syncthing
File: lib/osutil/lan.go (Go)

Code before:
// 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 osutil

import (
	"net"
)

func GetLans() ([]*net.IPNet, error) {
	ifs, err := net.Interfaces()
	if err != nil {
		return nil, err
	}
	addrs := []net.Addr{}
	for _, currentIf := range ifs {
		if currentIf.Flags&net.FlagUp != net.FlagUp {
			continue
		}
		currentAddrs, err := currentIf.Addrs()
		if err != nil {
			return nil, err
		}
		for _, currentAddr := range currentAddrs {
			addrs = append(addrs, currentAddr)
		}
	}

	nets := make([]*net.IPNet, 0, len(addrs))

	for _, addr := range addrs {
		net, ok := addr.(*net.IPNet)
		if ok {
			nets = append(nets, net)
		}
	}
	return nets, nil
}


Review comment (suggestion):
```suggestion
		addrs = append(addrs, currentAddrs...)
```
```suggestion
		for _, currentAddr := range currentAddrs {
			addrs = append(addrs, currentAddr)
		}
```

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

import (
	"net"
)

func GetLans() ([]*net.IPNet, error) {
	ifs, err := net.Interfaces()
	if err != nil {
		return nil, err
	}
	addrs := []net.Addr{}
	for _, currentIf := range ifs {
		if currentIf.Flags&net.FlagUp != net.FlagUp {
			continue
		}
		currentAddrs, err := currentIf.Addrs()
		if err != nil {
			return nil, err
		}
		addrs = append(addrs, currentAddrs...)
	}

	nets := make([]*net.IPNet, 0, len(addrs))

	for _, addr := range addrs {
		net, ok := addr.(*net.IPNet)
		if ok {
			nets = append(nets, net)
		}
	}
	return nets, nil
}
Repository: syncthing/syncthing
File: lib/model/folder_sendrecv.go (Go)

Code before:
		var caseErr *fs.ErrCaseConflict
		switch {
		case errors.As(serr, &caseErr):
			if caseErr.Real != source.Name {
				err = serr
				break
			}
			fallthrough // This is a case only rename
		case fs.IsNotExist(serr):
			if !ok || curTarget.IsDeleted() {
				break
			}
			scanChan <- target.Name
			err = errModified
		default:
			// We can't check whether the file changed as compared to the db,
			// do not delete.
			err = serr
		}
	case !ok:
		// Target appeared from nowhere
		scanChan <- target.Name
		err = errModified
	default:
		var fi protocol.FileInfo
		if fi, err = scanner.CreateFileInfo(stat, target.Name, f.mtimefs); err == nil {
			if !fi.IsEquivalentOptional(curTarget, protocol.FileInfoComparison{
				ModTimeWindow:   f.modTimeWindow,
				IgnorePerms:     f.IgnorePerms,
				IgnoreBlocks:    true,
				IgnoreFlags:     protocol.LocalAllFlags,
				IgnoreOwnership: !f.SyncOwnership,
			}) {
				// Target changed
				scanChan <- target.Name
				err = errModified
			}
		}
	}
	if err != nil {
		return err
	}

	tempName := fs.TempName(target.Name)

	if f.versioner != nil {
		err = f.CheckAvailableSpace(uint64(source.Size))
		if err == nil {
			err = osutil.Copy(f.CopyRangeMethod, f.mtimefs, f.mtimefs, source.Name, tempName)
			if err == nil {
				err = f.inWritableDir(f.versioner.Archive, source.Name)

Review comment (suggestion):
In these places we use the `SyncOwnership` bool instead of `ScanOwnership`, because it's about detecting whether the file is different from what we've synced previously. If we aren't syncing ownership we don't need to load ownership info because we're not going to compare it.

Code after:
		var caseErr *fs.ErrCaseConflict
		switch {
		case errors.As(serr, &caseErr):
			if caseErr.Real != source.Name {
				err = serr
				break
			}
			fallthrough // This is a case only rename
		case fs.IsNotExist(serr):
			if !ok || curTarget.IsDeleted() {
				break
			}
			scanChan <- target.Name
			err = errModified
		default:
			// We can't check whether the file changed as compared to the db,
			// do not delete.
			err = serr
		}
	case !ok:
		// Target appeared from nowhere
		scanChan <- target.Name
		err = errModified
	default:
		var fi protocol.FileInfo
		if fi, err = scanner.CreateFileInfo(stat, target.Name, f.mtimefs, f.SyncOwnership); err == nil {
			if !fi.IsEquivalentOptional(curTarget, protocol.FileInfoComparison{
				ModTimeWindow:   f.modTimeWindow,
				IgnorePerms:     f.IgnorePerms,
				IgnoreBlocks:    true,
				IgnoreFlags:     protocol.LocalAllFlags,
				IgnoreOwnership: !f.SyncOwnership,
			}) {
				// Target changed
				scanChan <- target.Name
				err = errModified
			}
		}
	}
	if err != nil {
		return err
	}

	tempName := fs.TempName(target.Name)

	if f.versioner != nil {
		err = f.CheckAvailableSpace(uint64(source.Size))
		if err == nil {
			err = osutil.Copy(f.CopyRangeMethod, f.mtimefs, f.mtimefs, source.Name, tempName)
			if err == nil {
				err = f.inWritableDir(f.versioner.Archive, source.Name)
Repository: syncthing/syncthing
File: cmd/syncthing/monitor.go (Go)

Code before:
		time.Sleep(restartPause)

		if first {
			// Let the next child process know that this is not the first time
			// it's starting up.
			childEnv = append(childEnv, "STRESTART=yes")
			first = false
		}
	}
}

func getBinary(args0 string) (string, error) {
	e, err := os.Executable()
	if err == nil {
		return e, nil
	}
	// Check if args0 cuts it
	_, err = exec.LookPath(args0)
	if err == nil {
		return args0, nil
	}
	// Works around a restriction added in go1.19 that executables in the
	// current directory are not resolved when specifying just an executable
	// name (like e.g. "syncthing")
	if !strings.ContainsRune(args0, os.PathSeparator) {
		e = "." + string(os.PathSeparator) + args0
		_, err := exec.LookPath(args0)
		if err == nil {
			return e, nil
		}
	}
	return "", fmt.Errorf("can't find executable")
}

func copyStderr(stderr io.Reader, dst io.Writer) {
	br := bufio.NewReader(stderr)

	var panicFd *os.File
	defer func() {
		if panicFd != nil {
			_ = panicFd.Close()
			maybeReportPanics()
		}
	}()

	for {
		line, err := br.ReadString('\n')
		if err != nil {
			return
		}


Review comment (suggestion):
I still think we should skip this entire thing, os.Executable() + exec.LookPath() should cover the reasonable scenarios imho. At least I don't see the scenario where it would be required or useful to also look separately in pwd, so despite not seeing how this would cause a problem by itself I don't think we should do it...

Code after:
		time.Sleep(restartPause)

		if first {
			// Let the next child process know that this is not the first time
			// it's starting up.
			childEnv = append(childEnv, "STRESTART=yes")
			first = false
		}
	}
}

func getBinary(args0 string) (string, error) {
	e, err := os.Executable()
	if err == nil {
		return e, nil
	}
	// Check if args0 cuts it
	e, lerr := exec.LookPath(args0)
	if lerr == nil {
		return e, nil
	}
	return "", err
}

func copyStderr(stderr io.Reader, dst io.Writer) {
	br := bufio.NewReader(stderr)

	var panicFd *os.File
	defer func() {
		if panicFd != nil {
			_ = panicFd.Close()
			maybeReportPanics()
		}
	}()

	for {
		line, err := br.ReadString('\n')
		if err != nil {
			return
		}

		if panicFd == nil {
			dst.Write([]byte(line))

			if strings.Contains(line, "SIGILL") {
				l.Warnln(`
*******************************************************************************
* Crash due to illegal instruction detected. This is most likely due to a CPU *
* incompatibility with the high performance hashing package. Switching to the *
* standard hashing package instead. Please report this issue at:              *
*                                                                             *
Repository: syncthing/syncthing
File: lib/versioner/simple.go (Go)

Code before:
func (v simple) Restore(filepath string, versionTime time.Time) error {
	return restoreFile(v.copyRangeMethod, v.versionsFs, v.folderFs, filepath, versionTime, TagFilename)
}

func (v simple) Clean(ctx context.Context) error {
	return clean(ctx, v.versionsFs, v.toRemove)
}

func (v simple) toRemove(versions []string, now time.Time) []string {
	var remove []string

	// The list of versions may or may not be properly sorted.
	sort.Strings(versions)

	// If the amount of elements exceeds the limit: the oldest elements are to be removed.
	if len(versions) > v.keep {
		remove = versions[:len(versions)-v.keep]
		versions = versions[len(versions)-v.keep:]
	}

	// If cleanoutDays is not a positive value then don't remove based on age.
	if v.cleanoutDays <= 0 {
		return remove
	}

	maxAge := int64(v.cleanoutDays * 24 * 60 * 60)

	// For the rest of the versions, elements which are too old are to be removed
	for _, version := range versions {
		versionTime, err := time.ParseInLocation(TimeFormat, extractTag(version), time.Local)
		if err != nil {
			l.Debugf("Versioner: file name %q is invalid: %v", version, err)
			continue
		}

		versionAge := int64(now.Sub(versionTime).Seconds())

		if versionAge > maxAge {
			remove = append(remove, version)
		}
	}

	return remove
}


Review comment (suggestion):
Better make this a duration directly,

```suggestion
	maxAge := time.duration(v.cleanoutDays) * 24 * time.Hour
```

Code after:
func (v simple) Restore(filepath string, versionTime time.Time) error {
	return restoreFile(v.copyRangeMethod, v.versionsFs, v.folderFs, filepath, versionTime, TagFilename)
}

func (v simple) Clean(ctx context.Context) error {
	return clean(ctx, v.versionsFs, v.toRemove)
}

func (v simple) toRemove(versions []string, now time.Time) []string {
	var remove []string

	// The list of versions may or may not be properly sorted.
	sort.Strings(versions)

	// If the amount of elements exceeds the limit: the oldest elements are to be removed.
	if len(versions) > v.keep {
		remove = versions[:len(versions)-v.keep]
		versions = versions[len(versions)-v.keep:]
	}

	// If cleanoutDays is not a positive value then don't remove based on age.
	if v.cleanoutDays <= 0 {
		return remove
	}

	maxAge := time.Duration(v.cleanoutDays) * 24 * time.Hour

	// For the rest of the versions, elements which are too old are to be removed
	for _, version := range versions {
		versionTime, err := time.ParseInLocation(TimeFormat, extractTag(version), time.Local)
		if err != nil {
			l.Debugf("Versioner: file name %q is invalid: %v", version, err)
			continue
		}

		if now.Sub(versionTime) > maxAge {
			remove = append(remove, version)
		}
	}

	return remove
}
Repository: syncthing/syncthing
File: lib/versioner/simple.go (Go)

Code before:
	// If the amount of elements exceeds the limit: the oldest elements are to be removed.
	if len(versions) > v.keep {
		remove = versions[:len(versions)-v.keep]
		versions = versions[len(versions)-v.keep:]
	}

	// If cleanoutDays is not a positive value then don't remove based on age.
	if v.cleanoutDays <= 0 {
		return remove
	}

	maxAge := int64(v.cleanoutDays * 24 * 60 * 60)

	// For the rest of the versions, elements which are too old are to be removed
	for _, version := range versions {
		versionTime, err := time.ParseInLocation(TimeFormat, extractTag(version), time.Local)
		if err != nil {
			l.Debugf("Versioner: file name %q is invalid: %v", version, err)
			continue
		}

		versionAge := int64(now.Sub(versionTime).Seconds())

		if versionAge > maxAge {
			remove = append(remove, version)
		}
	}

	return remove
}


Review comment (suggestion):
With the maxAge from above, this can be simpler:

```suggestion
		if now.Sub(versionTime) > maxAge {
			remove = append(remove, version)
		}
```

Code after:
	// If the amount of elements exceeds the limit: the oldest elements are to be removed.
	if len(versions) > v.keep {
		remove = versions[:len(versions)-v.keep]
		versions = versions[len(versions)-v.keep:]
	}

	// If cleanoutDays is not a positive value then don't remove based on age.
	if v.cleanoutDays <= 0 {
		return remove
	}

	maxAge := time.Duration(v.cleanoutDays) * 24 * time.Hour

	// For the rest of the versions, elements which are too old are to be removed
	for _, version := range versions {
		versionTime, err := time.ParseInLocation(TimeFormat, extractTag(version), time.Local)
		if err != nil {
			l.Debugf("Versioner: file name %q is invalid: %v", version, err)
			continue
		}

		if now.Sub(versionTime) > maxAge {
			remove = append(remove, version)
		}
	}

	return remove
}
Repository: syncthing/syncthing
File: lib/versioner/trashcan_test.go (Go)

Code before:
	buf, err := io.ReadAll(fd)
	if err != nil {
		t.Fatal(err)
	}
	return string(buf)
}

func writeFile(t *testing.T, filesystem fs.Filesystem, name, content string) {
	fd, err := filesystem.OpenFile(name, fs.OptReadWrite|fs.OptCreate, 0777)
	if err != nil {
		t.Fatal(err)
	}
	defer fd.Close()
	if err := fd.Truncate(int64(len(content))); err != nil {
		t.Fatal(err)
	}

	if n, err := fd.Write([]byte(content)); err != nil || n != len(content) {
		t.Fatal(n, len(content), err)
	}
}

func TestTrashcanCleanOut(t *testing.T) {
	cfg := config.FolderConfiguration{
		FilesystemType: fs.FilesystemTypeBasic,
		Path:           "testdata",
		Versioning: config.VersioningConfiguration{
			Params: map[string]string{
				"cleanoutDays": "7",
			},
		},
	}

	v := newTrashcan(cfg)

	var testcases = map[string]bool{
		"testdata/.stversions/file1":                     false,
		"testdata/.stversions/file2":                     true,
		"testdata/.stversions/keep1/file1":               false,
		"testdata/.stversions/keep1/file2":               false,
		"testdata/.stversions/keep2/file1":               false,
		"testdata/.stversions/keep2/file2":               true,
		"testdata/.stversions/keep3/keepsubdir/file1":    false,
		"testdata/.stversions/remove/file1":              true,
		"testdata/.stversions/remove/file2":              true,
		"testdata/.stversions/remove/removesubdir/file1": true,
	}

	t.Run(fmt.Sprintf("trashcan versioner trashcan clean up"), func(t *testing.T) {
		os.RemoveAll("testdata")
		defer os.RemoveAll("testdata")

Review comment (suggestion):
There's some legacy stuff here with tests that operate in `testdata`, but for new tests we should use either `fs.FakeFS` or a tempdir from `t.TempDir()`

Code after:
	buf, err := io.ReadAll(fd)
	if err != nil {
		t.Fatal(err)
	}
	return string(buf)
}

func writeFile(t *testing.T, filesystem fs.Filesystem, name, content string) {
	fd, err := filesystem.OpenFile(name, fs.OptReadWrite|fs.OptCreate, 0777)
	if err != nil {
		t.Fatal(err)
	}
	defer fd.Close()
	if err := fd.Truncate(int64(len(content))); err != nil {
		t.Fatal(err)
	}

	if n, err := fd.Write([]byte(content)); err != nil || n != len(content) {
		t.Fatal(n, len(content), err)
	}
}

func TestTrashcanCleanOut(t *testing.T) {
	testDir := t.TempDir()

	cfg := config.FolderConfiguration{
		FilesystemType: fs.FilesystemTypeBasic,
		Path:           testDir,
		Versioning: config.VersioningConfiguration{
			Params: map[string]string{
				"cleanoutDays": "7",
			},
		},
	}

	fs := cfg.Filesystem(nil)

	v := newTrashcan(cfg)

	var testcases = map[string]bool{
		".stversions/file1":                     false,
		".stversions/file2":                     true,
		".stversions/keep1/file1":               false,
		".stversions/keep1/file2":               false,
		".stversions/keep2/file1":               false,
		".stversions/keep2/file2":               true,
		".stversions/keep3/keepsubdir/file1":    false,
		".stversions/remove/file1":              true,
		".stversions/remove/file2":              true,
		".stversions/remove/removesubdir/file1": true,
	}
Repository: syncthing/syncthing
File: lib/versioner/trashcan_test.go (Go)

Code before:
				"cleanoutDays": "7",
			},
		},
	}

	fs := cfg.Filesystem(nil)

	v := newTrashcan(cfg)

	var testcases = map[string]bool{
		".stversions/file1":                     false,
		".stversions/file2":                     true,
		".stversions/keep1/file1":               false,
		".stversions/keep1/file2":               false,
		".stversions/keep2/file1":               false,
		".stversions/keep2/file2":               true,
		".stversions/keep3/keepsubdir/file1":    false,
		".stversions/remove/file1":              true,
		".stversions/remove/file2":              true,
		".stversions/remove/removesubdir/file1": true,
	}

	t.Run(fmt.Sprintf("trashcan versioner trashcan clean up"), func(t *testing.T) {
		fs.RemoveAll("testdata")
		defer fs.RemoveAll("testdata")

		oldTime := time.Now().Add(-8 * 24 * time.Hour)
		for file, shouldRemove := range testcases {
			fs.MkdirAll(filepath.Dir(file), 0777)

			writeFile(t, fs, file, "some content")

			if shouldRemove {
				if err := fs.Chtimes(file, oldTime, oldTime); err != nil {
					t.Fatal(err)
				}
			}

		}

		if err := v.Clean(context.Background()); err != nil {
			t.Fatal(err)
		}

		for file, shouldRemove := range testcases {
			_, err := fs.Lstat(file)
			if shouldRemove && !os.IsNotExist(err) {
				t.Error(file, "should have been removed")
			} else if !shouldRemove && err != nil {
				t.Error(file, "should not have been removed")
			}

Review comment (suggestion):
```suggestion
```

remove as we no longer use testdata here

Code after:
			},
		},
	}

	fs := cfg.Filesystem(nil)

	v := newTrashcan(cfg)

	var testcases = map[string]bool{
		".stversions/file1":                     false,
		".stversions/file2":                     true,
		".stversions/keep1/file1":               false,
		".stversions/keep1/file2":               false,
		".stversions/keep2/file1":               false,
		".stversions/keep2/file2":               true,
		".stversions/keep3/keepsubdir/file1":    false,
		".stversions/remove/file1":              true,
		".stversions/remove/file2":              true,
		".stversions/remove/removesubdir/file1": true,
	}

	t.Run(fmt.Sprintf("trashcan versioner trashcan clean up"), func(t *testing.T) {
		oldTime := time.Now().Add(-8 * 24 * time.Hour)
		for file, shouldRemove := range testcases {
			fs.MkdirAll(filepath.Dir(file), 0777)

			writeFile(t, fs, file, "some content")

			if shouldRemove {
				if err := fs.Chtimes(file, oldTime, oldTime); err != nil {
					t.Fatal(err)
				}
			}
		}

		if err := v.Clean(context.Background()); err != nil {
			t.Fatal(err)
		}

		for file, shouldRemove := range testcases {
			_, err := fs.Lstat(file)
			if shouldRemove && !os.IsNotExist(err) {
				t.Error(file, "should have been removed")
			} else if !shouldRemove && err != nil {
				t.Error(file, "should not have been removed")
			}
		}

		if _, err := fs.Lstat(".stversions/keep3"); os.IsNotExist(err) {
			t.Error("directory with non empty subdirs should not be removed")
		}
Repository: syncthing/syncthing
File: script/translate.go (Go)

Code before:
			log.Print("\t" + v)
		}
	}
	for c := n.FirstChild; c != nil; c = c.NextSibling {
		if translate {
			inTranslate(c, filename, keyname)
		} else {
			generalNode(c, filename)
		}
	}
}

func inTranslate(n *html.Node, filename string, keyname string) {
	if n.Type == html.TextNode {
		translation(n.Data, keyname)
	} else {
		log.Println("translate node with non-text child < (" + filename + ")")
		log.Println(n)
	}
	if n.FirstChild != nil {
		log.Println("translate node has children (" + filename + "):")
		log.Println(n.Data)
	}
}

func translation(v string, k string) {
	v = strings.TrimSpace(v)
	if _, ok := trans[v]; !ok {
		av := strings.Replace(v, "{%", "{{", -1)
		av = strings.Replace(av, "%}", "}}", -1)
		if len(k) > 0 {
			trans[k] = av
		} else {
			trans[v] = av
		}
	}
}

func walkerFor(basePath string) filepath.WalkFunc {
	return func(name string, info os.FileInfo, err error) error {
		if err != nil {
			return err
		}

		if !info.Mode().IsRegular() {
			return nil
		}
		fd, err := os.Open(name)
		if err != nil {
			log.Fatal(err)
		}

Review comment (suggestion):
I don't think we need a separate key here.  The `v` argument holds the key, which also serves as the initial translation unless there is a better one.

Code after:
		if translate {
			inTranslate(c, filename)
		} else {
			generalNode(c, filename)
		}
	}
}

func inTranslate(n *html.Node, filename string) {
	if n.Type == html.TextNode {
		translation(n.Data)
	} else {
		log.Println("translate node with non-text child < (" + filename + ")")
		log.Println(n)
	}
	if n.FirstChild != nil {
		log.Println("translate node has children (" + filename + "):")
		log.Println(n.Data)
	}
}

func translation(v string) {
	v = strings.TrimSpace(v)
	if _, ok := trans[v]; !ok {
		av := strings.Replace(v, "{%", "{{", -1)
		av = strings.Replace(av, "%}", "}}", -1)
		trans[v] = av
	}
}

func walkerFor(basePath string) filepath.WalkFunc {
	return func(name string, info os.FileInfo, err error) error {
		if err != nil {
			return err
		}

		if !info.Mode().IsRegular() {
			return nil
		}
		fd, err := os.Open(name)
		if err != nil {
			log.Fatal(err)
		}
		defer fd.Close()
		switch filepath.Ext(name) {
		case ".html":
			doc, err := html.Parse(fd)
			if err != nil {
				log.Fatal(err)
			}
			generalNode(doc, filepath.Base(name))