Merge pull request #3949 from zramsay/fix/codebase-consistency

apply the megacheck code vetting tool for idiomatic go

.codeclimate.yml

@@ -11,6 +11,12 @@ exclude_paths:
 engines:
   fixme:
     enabled: true
+    config:
+      strings:
+      - FIXME
+      - HACK
+      - XXX
+      - BUG
   golint:
     enabled: true
   govet:

Rules.mk

@@ -67,7 +67,6 @@ include $(dir)/Rules.mk
 #     core targets     #
 # -------------------- #
 
-
 build: $(TGT_BIN)
 .PHONY: build
 
@@ -143,6 +142,7 @@ help:
 	@echo '  test_go_short'
 	@echo '  test_go_expensive'
 	@echo '  test_go_race'
+	@echo '  test_go_megacheck'	 - Run the `megacheck` vetting tool
 	@echo '  test_sharness_short'
 	@echo '  test_sharness_expensive'
 	@echo '  test_sharness_race'

blocks/blocks_test.go

@@ -91,9 +91,8 @@ func TestManualHash(t *testing.T) {
 
 	u.Debug = true
 
-	block, err = NewBlockWithCid(data, c)
+	_, err = NewBlockWithCid(data, c)
 	if err != ErrWrongHash {
 		t.Fatal(err)
 	}
-
 }

blocks/blockstore/arc_cache_test.go

@@ -30,7 +30,7 @@ func testArcCached(ctx context.Context, bs Blockstore) (*arccache, error) {
 func createStores(t *testing.T) (*arccache, Blockstore, *callbackDatastore) {
 	cd := &callbackDatastore{f: func() {}, ds: ds.NewMapDatastore()}
 	bs := NewBlockstore(syncds.MutexWrap(cd))
-	arc, err := testArcCached(nil, bs)
+	arc, err := testArcCached(context.TODO(), bs)
 	if err != nil {
 		t.Fatal(err)
 	}

blocks/blockstore/blockstore.go

@@ -102,10 +102,6 @@ func NewBlockstore(d ds.Batching) Blockstore {
 type blockstore struct {
 	datastore ds.Batching
 
-	lk      sync.RWMutex
-	gcreq   int32
-	gcreqlk sync.Mutex
-
 	rehash bool
 }
 
@@ -246,9 +242,8 @@ func NewGCLocker() GCLocker {
 }
 
 type gclocker struct {
-	lk      sync.RWMutex
-	gcreq   int32
-	gcreqlk sync.Mutex
+	lk    sync.RWMutex
+	gcreq int32
 }
 
 // Unlocker represents an object which can Unlock

blocks/blockstore/bloom_cache.go

@@ -118,7 +118,7 @@ func (b *bloomcache) hasCached(k *cid.Cid) (has bool, ok bool) {
 	}
 	if b.BloomActive() {
 		blr := b.bloom.HasTS(k.Bytes())
-		if blr == false { // not contained in bloom is only conclusive answer bloom gives
+		if !blr { // not contained in bloom is only conclusive answer bloom gives
 			b.hits.Inc()
 			return false, true
 		}

blocks/blockstore/bloom_cache_test.go

@@ -34,6 +34,9 @@ func TestPutManyAddsToBloom(t *testing.T) {
 	defer cancel()
 
 	cachedbs, err := testBloomCached(ctx, bs)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	select {
 	case <-cachedbs.rebuildChan:
@@ -49,15 +52,15 @@ func TestPutManyAddsToBloom(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if has == false {
+	if !has {
 		t.Fatal("added block is reported missing")
 	}
 
 	has, err = cachedbs.Has(block2.Cid())
 	if err != nil {
 		t.Fatal(err)
 	}
-	if has == true {
+	if has {
 		t.Fatal("not added block is reported to be in blockstore")
 	}
 }

blocks/blockstore/caching_test.go

@@ -1,26 +1,29 @@
 package blockstore
 
-import "testing"
+import (
+	"context"
+	"testing"
+)
 
 func TestCachingOptsLessThanZero(t *testing.T) {
 	opts := DefaultCacheOpts()
 	opts.HasARCCacheSize = -1
 
-	if _, err := CachedBlockstore(nil, nil, opts); err == nil {
+	if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
 		t.Error("wrong ARC setting was not detected")
 	}
 
 	opts = DefaultCacheOpts()
 	opts.HasBloomFilterSize = -1
 
-	if _, err := CachedBlockstore(nil, nil, opts); err == nil {
+	if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
 		t.Error("negative bloom size was not detected")
 	}
 
 	opts = DefaultCacheOpts()
 	opts.HasBloomFilterHashes = -1
 
-	if _, err := CachedBlockstore(nil, nil, opts); err == nil {
+	if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
 		t.Error("negative hashes setting was not detected")
 	}
 }
@@ -29,7 +32,7 @@ func TestBloomHashesAtZero(t *testing.T) {
 	opts := DefaultCacheOpts()
 	opts.HasBloomFilterHashes = 0
 
-	if _, err := CachedBlockstore(nil, nil, opts); err == nil {
+	if _, err := CachedBlockstore(context.TODO(), nil, opts); err == nil {
 		t.Error("zero hashes setting with positive size was not detected")
 	}
 }

blocks/set/set.go

@@ -4,14 +4,11 @@
 package set
 
 import (
-	logging "gx/ipfs/QmSpJByNKFX1sCsHBEp3R73FL4NF6FnQTEGyNAXHm2GS52/go-log"
 	cid "gx/ipfs/QmYhQaCYEcaPPjxJX7YcPcVKkQfRy6sJ7B3XmGFk82XYdQ/go-cid"
 
 	"github.com/ipfs/go-ipfs/blocks/bloom"
 )
 
-var log = logging.Logger("blockset")
-
 // BlockSet represents a mutable set of blocks CIDs.
 type BlockSet interface {
 	AddBlock(*cid.Cid)

blocks/set/set_test.go

@@ -25,15 +25,15 @@ func exampleKeys() []*cid.Cid {
 func checkSet(set BlockSet, keySlice []*cid.Cid, t *testing.T) {
 	for i, key := range keySlice {
 		if i&tReAdd == 0 {
-			if set.HasKey(key) == false {
+			if !set.HasKey(key) {
 				t.Error("key should be in the set")
 			}
 		} else if i&tRemove == 0 {
-			if set.HasKey(key) == true {
+			if set.HasKey(key) {
 				t.Error("key shouldn't be in the set")
 			}
 		} else if i&tAdd == 0 {
-			if set.HasKey(key) == false {
+			if !set.HasKey(key) {
 				t.Error("key should be in the set")
 			}
 		}
@@ -70,7 +70,7 @@ func TestSetWorks(t *testing.T) {
 	bloom := set.GetBloomFilter()
 
 	for _, key := range addedKeys {
-		if bloom.Find(key.Bytes()) == false {
+		if !bloom.Find(key.Bytes()) {
 			t.Error("bloom doesn't contain expected key")
 		}
 	}

blockservice/blockservice.go

@@ -172,7 +172,7 @@ func (s *blockService) GetBlock(ctx context.Context, c *cid.Cid) (blocks.Block,
 // the returned channel.
 // NB: No guarantees are made about order.
 func (s *blockService) GetBlocks(ctx context.Context, ks []*cid.Cid) <-chan blocks.Block {
-	out := make(chan blocks.Block, 0)
+	out := make(chan blocks.Block)
 	go func() {
 		defer close(out)
 		var misses []*cid.Cid

cmd/ipfs/daemon.go

@@ -201,11 +201,9 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
 	ctx := req.InvocContext()
 
 	go func() {
-		select {
-		case <-req.Context().Done():
-			fmt.Println("Received interrupt signal, shutting down...")
-			fmt.Println("(Hit ctrl-c again to force-shutdown the daemon.)")
-		}
+		<-req.Context().Done()
+		fmt.Println("Received interrupt signal, shutting down...")
+		fmt.Println("(Hit ctrl-c again to force-shutdown the daemon.)")
 	}()
 
 	// check transport encryption flag.
@@ -418,7 +416,6 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
 			res.SetError(err, cmds.ErrNormal)
 		}
 	}
-	return
 }
 
 // serveHTTPApi collects options, creates listener, prints status message and starts serving requests

cmd/ipfs/ipfs.go

@@ -44,12 +44,6 @@ func init() {
 	}
 }
 
-// isLocal returns true if the command should only be run locally (not sent to daemon), otherwise false
-func isLocal(cmd *cmds.Command) bool {
-	_, found := localMap[cmd]
-	return found
-}
-
 // NB: when necessary, properties are described using negatives in order to
 // provide desirable defaults
 type cmdDetails struct {
@@ -85,11 +79,10 @@ func (d *cmdDetails) Loggable() map[string]interface{} {
 	}
 }
 
-func (d *cmdDetails) usesConfigAsInput() bool        { return !d.doesNotUseConfigAsInput }
-func (d *cmdDetails) doesNotPreemptAutoUpdate() bool { return !d.preemptsAutoUpdate }
-func (d *cmdDetails) canRunOnClient() bool           { return !d.cannotRunOnClient }
-func (d *cmdDetails) canRunOnDaemon() bool           { return !d.cannotRunOnDaemon }
-func (d *cmdDetails) usesRepo() bool                 { return !d.doesNotUseRepo }
+func (d *cmdDetails) usesConfigAsInput() bool { return !d.doesNotUseConfigAsInput }
+func (d *cmdDetails) canRunOnClient() bool    { return !d.cannotRunOnClient }
+func (d *cmdDetails) canRunOnDaemon() bool    { return !d.cannotRunOnDaemon }
+func (d *cmdDetails) usesRepo() bool          { return !d.doesNotUseRepo }
 
 // "What is this madness!?" you ask. Our commands have the unfortunate problem of
 // not being able to run on all the same contexts. This map describes these

cmd/ipfs/main.go

@@ -37,17 +37,12 @@ import (
 // log is the command logger
 var log = logging.Logger("cmd/ipfs")
 
-var (
-	errUnexpectedApiOutput = errors.New("api returned unexpected output")
-	errApiVersionMismatch  = errors.New("api version mismatch")
-	errRequestCanceled     = errors.New("request canceled")
-)
+var errRequestCanceled = errors.New("request canceled")
 
 const (
 	EnvEnableProfiling = "IPFS_PROF"
 	cpuProfile         = "ipfs.cpuprof"
 	heapProfile        = "ipfs.memprof"
-	errorFormat        = "ERROR: %v\n\n"
 )
 
 type cmdInvocation struct {
@@ -492,7 +487,7 @@ func startProfiling() (func(), error) {
 	}
 	pprof.StartCPUProfile(ofi)
 	go func() {
-		for _ = range time.NewTicker(time.Second * 30).C {
+		for range time.NewTicker(time.Second * 30).C {
 			err := writeHeapProfileToFile()
 			if err != nil {
 				log.Error(err)
@@ -546,7 +541,7 @@ func (ih *IntrHandler) Handle(handler func(count int, ih *IntrHandler), sigs ...
 	go func() {
 		defer ih.wg.Done()
 		count := 0
-		for _ = range ih.sig {
+		for range ih.sig {
 			count++
 			handler(count, ih)
 		}

cmd/ipfswatch/main.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"os/signal"
 	"path/filepath"
+	"syscall"
 
 	commands "github.com/ipfs/go-ipfs/commands"
 	core "github.com/ipfs/go-ipfs/core"
@@ -99,7 +100,7 @@ func run(ipfsPath, watchPath string) error {
 	}
 
 	interrupts := make(chan os.Signal)
-	signal.Notify(interrupts, os.Interrupt, os.Kill)
+	signal.Notify(interrupts, os.Interrupt, syscall.SIGTERM)
 
 	for {
 		select {
@@ -167,10 +168,7 @@ func addTree(w *fsnotify.Watcher, root string) error {
 		}
 		return nil
 	})
-	if err != nil {
-		return err
-	}
-	return nil
+	return err
 }
 
 func IsDirectory(path string) (bool, error) {

commands/cli/helptext.go

@@ -16,9 +16,6 @@ const (
 	variadicArg = "%v..."
 	shortFlag   = "-%v"
 	longFlag    = "--%v"
-	optionType  = "(%v)"
-
-	whitespace = "\r\n\t "
 
 	indentStr = "  "
 )
@@ -295,9 +292,7 @@ func optionText(cmd ...*cmds.Command) []string {
 	// get a slice of the options we want to list out
 	options := make([]cmds.Option, 0)
 	for _, c := range cmd {
-		for _, opt := range c.Options {
-			options = append(options, opt)
-		}
+		options = append(options, c.Options...)
 	}
 
 	// add option names to output (with each name aligned)
@@ -427,13 +422,6 @@ func align(lines []string) []string {
 	return lines
 }
 
-func indent(lines []string, prefix string) []string {
-	for i, line := range lines {
-		lines[i] = prefix + indentString(line, prefix)
-	}
-	return lines
-}
-
 func indentString(line string, prefix string) string {
 	return prefix + strings.Replace(line, "\n", "\n"+prefix, -1)
 }

commands/cli/parse.go

@@ -59,11 +59,8 @@ func Parse(input []string, stdin *os.File, root *cmds.Command) (cmds.Request, *c
 	}
 
 	err = cmd.CheckArguments(req)
-	if err != nil {
-		return req, cmd, path, err
-	}
 
-	return req, cmd, path, nil
+	return req, cmd, path, err
 }
 
 func ParseArgs(req cmds.Request, inputs []string, stdin *os.File, argDefs []cmds.Argument, root *cmds.Command) ([]string, []files.File, error) {

commands/cli/parse_test.go

@@ -204,7 +204,7 @@ func TestArgumentParsing(t *testing.T) {
 
 	test := func(cmd words, f *os.File, res words) {
 		if f != nil {
-			if _, err := f.Seek(0, os.SEEK_SET); err != nil {
+			if _, err := f.Seek(0, io.SeekStart); err != nil {
 				t.Fatal(err)
 			}
 		}

commands/command_test.go

@@ -3,7 +3,6 @@ package commands
 import "testing"
 
 func noop(req Request, res Response) {
-	return
 }
 
 func TestOptionValidation(t *testing.T) {

commands/files/multipartfile.go

@@ -10,7 +10,6 @@ import (
 
 const (
 	multipartFormdataType = "multipart/form-data"
-	multipartMixedType    = "multipart/mixed"
 
 	applicationDirectory = "application/x-directory"
 	applicationSymlink   = "application/symlink"