fix: address gocritic issues (#127)

* fix: address gocritic issues

Example of the weird logic of slices:

```go
package main

import "fmt"

type Data struct {
  res []int
}

func (d *Data) SetRes(res []int) {
  d.res = res
}

func (d *Data) Res() []int {
  return d.res
}

func main() {
  // New struct
  data := Data{}
  // Base data
  hist := []int{1, 2, 3, 4, 5, 6, 7}

  // Set res to be only the first 5 element of hist.
  data.SetRes(hist[:5])

  fmt.Printf("[before append] hist (%d): %+v\n", len(hist), hist)

  // Surely, appending to data.Res() and save it as 'nest' should not
  // change 'hist', right?
  nest := append(data.Res(), 8)

  // Right?
  fmt.Printf("[after append]  hist (%d): %+v\n", len(hist), hist)
  fmt.Printf("[after append]  nest (%d): %+v\n", len(nest), nest)
}
```

Output:

```
go run main.go
[before append] hist (7): [1 2 3 4 5 6 7]
[after append]  hist (7): [1 2 3 4 5 8 7]
[after append]  nest (6): [1 2 3 4 5 8]
```

For example:
```go
extra = append(c.GetDescriptor().Resources, *res)
```

This line can lead to issues as soon as we use partial slices somewhere along
the chain, it can change this in the `ComponentDescriptor` because
`GetDescriptor` returns with a pointer.

Closes #85

* put o.GetHistory() back into a variable

* fix history copy by introducing appropriate method

Co-authored-by: Uwe Krueger <[email protected]>

.golangci.yaml

@@ -15,10 +15,6 @@ linters:
 
     # Logical next step
     - forcetypeassert  # Priority: that can lead to serious crashes.
-    - gocritic         # Priority:
-                       #   - security issues (slice copy)
-                       #   - unexpected behavior
-                       #   - Go practices ('p += k' and not 'p = p + k')
     - gosec            # Priority: Security (as the name implies)
     - revive           # Too many issues.
                        #   It's important to at least address:

cmds/ocm/app/app.go

@@ -243,10 +243,8 @@ func (o *CLIOptions) Complete() error {
 	}
 	if len(attrs) != 0 {
 		o.Context.CredentialsContext().SetCredentialsForConsumer(id, credentials.NewCredentials(attrs))
-	} else {
-		if len(id) != 0 {
-			return errors.Newf("empty credential attribute set for %s", id.String())
-		}
+	} else if len(id) != 0 {
+		return errors.Newf("empty credential attribute set for %s", id.String())
 	}
 
 	set, err := common2.ParseLabels(o.Settings, "attribute setting")

cmds/ocm/commands/ocicmds/common/handlers/artefacthdlr/clean.go

@@ -60,7 +60,7 @@ func clean(iterable data.Iterable) data.Iterable {
 		d := depth[dig]
 		if l == 0 && l < d && (e.Spec.Tag == nil || *e.Spec.Tag == tags[dig]) {
 			j := i + 1
-			prefix := append(e.History, common.NewNameVersion("", dig.String()))
+			prefix := e.History.Append(common.NewNameVersion("", dig.String()))
 			for ; j < len(data) && data[j].(*Object).History.HasPrefix(prefix); j++ {
 			}
 			data = append(data[:i], data[j:]...)

cmds/ocm/commands/ocmcmds/common/inputs/types/spiff/spec.go

@@ -67,10 +67,8 @@ func (s *Spec) Validate(fldPath *field.Path, ctx clictx.Context, inputFilePath s
 		fileInfo, filePath, err := inputs.FileInfo(ctx, v, inputFilePath)
 		if err != nil {
 			allErrs = append(allErrs, field.Invalid(pathField, filePath, err.Error()))
-		} else {
-			if !fileInfo.Mode().IsRegular() {
-				allErrs = append(allErrs, field.Invalid(pathField, filePath, "no regular file"))
-			}
+		} else if !fileInfo.Mode().IsRegular() {
+			allErrs = append(allErrs, field.Invalid(pathField, filePath, "no regular file"))
 		}
 	}
 	return allErrs

cmds/ocm/commands/ocmcmds/common/resources.go

@@ -326,11 +326,9 @@ func Validate(r *ResourceInput, ctx clictx.Context, inputFilePath string) error
 					}
 					raw, _ := r.Access.GetRaw()
 					allErrs = append(allErrs, field.Invalid(fldPath.Child("access"), string(raw), err.Error()))
-				} else {
-					if acc.(ocm.AccessSpec).IsLocal(ctx.OCMContext()) {
-						kind := runtime.ObjectVersionedType(r.Access.ObjectType).GetKind()
-						allErrs = append(allErrs, field.Invalid(fldPath.Child("access", "type"), kind, "local access no possible"))
-					}
+				} else if acc.(ocm.AccessSpec).IsLocal(ctx.OCMContext()) {
+					kind := runtime.ObjectVersionedType(r.Access.ObjectType).GetKind()
+					allErrs = append(allErrs, field.Invalid(fldPath.Child("access", "type"), kind, "local access no possible"))
 				}
 			}
 		}

cmds/ocm/commands/ocmcmds/references/get/cmd.go

@@ -29,7 +29,7 @@ import (
 	"github.com/open-component-model/ocm/cmds/ocm/pkg/output"
 	"github.com/open-component-model/ocm/cmds/ocm/pkg/processing"
 	"github.com/open-component-model/ocm/cmds/ocm/pkg/utils"
-	gcommom "github.com/open-component-model/ocm/pkg/common"
+	gcommon "github.com/open-component-model/ocm/pkg/common"
 	"github.com/open-component-model/ocm/pkg/contexts/clictx"
 	"github.com/open-component-model/ocm/pkg/contexts/ocm"
 	metav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1"
@@ -99,9 +99,9 @@ outer:
 	for i := 0; i < len(slice); i++ {
 		o := slice[i]
 		e := common.Elem(o)
-		key := gcommom.NewNameVersion(e.ComponentName, e.Version)
+		key := gcommon.NewNameVersion(e.ComponentName, e.Version)
 		hist := o.GetHistory()
-		nested := append(hist, key)
+		nested := hist.Append(key)
 		var j int
 		for j = i + 1; j < len(slice); j++ {
 			n := slice[j]

cmds/ocm/commands/ocmcmds/resources/add/resources.go

@@ -54,10 +54,8 @@ func (ResourceSpecHandler) Set(v ocm.ComponentVersionAccess, r common.Resource,
 	if spec.Relation == metav1.LocalRelation {
 		if vers == "" || vers == ComponentVersionTag {
 			vers = v.GetVersion()
-		} else {
-			if vers != v.GetVersion() {
-				return errors.Newf("local resource %q (%s) has non-matching version %q", spec.Name, r.Source(), vers)
-			}
+		} else if vers != v.GetVersion() {
+			return errors.Newf("local resource %q (%s) has non-matching version %q", spec.Name, r.Source(), vers)
 		}
 	}
 	if vers == ComponentVersionTag {

cmds/ocm/commands/ocmcmds/resources/download/cmd.go

@@ -229,10 +229,8 @@ func (d *action) Save(o *elemhdlr.Object, f string) error {
 		if err != nil {
 			return err
 		}
-	} else {
-		if eff != f && pathIn {
-			out.Outf(d.opts.Context, "output path %q changed to %q by downloader", f, eff)
-		}
+	} else if eff != f && pathIn {
+		out.Outf(d.opts.Context, "output path %q changed to %q by downloader", f, eff)
 	}
 	return nil
 }

cmds/ocm/commands/ocmcmds/sources/add/resources.go

@@ -83,11 +83,9 @@ func (r *ResourceSpec) Validate(ctx clictx.Context, input *common.ResourceInput)
 			if err != nil {
 				raw, _ := r.Access.GetRaw()
 				allErrs = append(allErrs, field.Invalid(fldPath.Child("access"), string(raw), err.Error()))
-			} else {
-				if acc.(ocm.AccessSpec).IsLocal(ctx.OCMContext()) {
-					kind := runtime.ObjectVersionedType(r.Access.ObjectType).GetKind()
-					allErrs = append(allErrs, field.Invalid(fldPath.Child("access", "type"), kind, "local access no possible"))
-				}
+			} else if acc.(ocm.AccessSpec).IsLocal(ctx.OCMContext()) {
+				kind := runtime.ObjectVersionedType(r.Access.ObjectType).GetKind()
+				allErrs = append(allErrs, field.Invalid(fldPath.Child("access", "type"), kind, "local access no possible"))
 			}
 		}
 	}

cmds/ocm/pkg/output/table.go

@@ -41,10 +41,8 @@ func FormatTable(ctx Context, gap string, data [][]string) {
 		for i, col := range row {
 			if i >= len(columns) {
 				columns = append(columns, len(col))
-			} else {
-				if columns[i] < len(col) {
-					columns[i] = len(col)
-				}
+			} else if columns[i] < len(col) {
+				columns[i] = len(col)
 			}
 			if len(col) > max {
 				max = len(col)

cmds/ocm/pkg/processing/chain.go

@@ -55,10 +55,8 @@ func (this *_ProcessChain) new(p *_ProcessChain, creator stepCreator) *_ProcessC
 	if p != nil {
 		if p.creator != nil {
 			this.parent = p
-		} else {
-			if p.parent != nil {
-				this.parent = p.parent
-			}
+		} else if p.parent != nil {
+			this.parent = p.parent
 		}
 	}
 	if this.parent != nil && creator == nil {
@@ -129,10 +127,8 @@ func (this *_ProcessChain) Process(data data.Iterable) ProcessingResult {
 	p, ok := data.(ProcessingResult)
 	if this.parent != nil {
 		p = this.parent.Process(data)
-	} else {
-		if !ok {
-			p = Process(data)
-		}
+	} else if !ok {
+		p = Process(data)
 	}
 	return this.step(p)
 }

cmds/ocm/pkg/utils/command.go

@@ -112,7 +112,7 @@ func SetupCommand(ocmcmd OCMCommand, names ...string) *cobra.Command {
 		return err
 	}
 	if u, ok := ocmcmd.(options.Usage); ok {
-		c.Long = c.Long + u.Usage()
+		c.Long += u.Usage()
 	}
 	ocmcmd.AddFlags(c.Flags())
 	return c

pkg/cobrautils/links.go

@@ -27,7 +27,7 @@ func LinkForCmd(cmd *cobra.Command) string {
 
 func LinkForPath(path string) string {
 	link := path + ".md"
-	link = strings.Replace(link, " ", "_", -1)
+	link = strings.ReplaceAll(link, " ", "_")
 	return link
 }
 

pkg/common/accessobj/filesystemaccess.go

@@ -119,10 +119,8 @@ func (a *FileSystemBlobAccess) AddBlob(blob accessio.BlobAccess) error {
 
 	if ok, err := vfs.FileExists(a.base.GetFileSystem(), path); ok {
 		return nil
-	} else {
-		if err != nil {
-			return fmt.Errorf("failed to check if '%s' file exists: %w", path, err)
-		}
+	} else if err != nil {
+		return fmt.Errorf("failed to check if '%s' file exists: %w", path, err)
 	}
 
 	r, err := blob.Reader()

pkg/common/accessobj/format.go

@@ -133,17 +133,18 @@ func DefaultOpenOptsFileHandling(kind string, info *AccessObjectInfo, acc Access
 	var file vfs.File
 	var err error
 	var closer Closer
-	if opts.Reader != nil {
+	switch {
+	case opts.Reader != nil:
 		reader = opts.Reader
 		defer opts.Reader.Close()
-	} else if opts.File == nil {
+	case opts.File == nil:
 		// we expect that the path point to a tar
 		file, err = opts.PathFileSystem.Open(path)
 		if err != nil {
 			return nil, fmt.Errorf("unable to open %s from %s: %w", kind, path, err)
 		}
 		defer file.Close()
-	} else {
+	default:
 		file = opts.File
 	}
 	if file != nil {

pkg/common/history.go

@@ -85,6 +85,13 @@ func (h *History) Add(kind string, nv NameVersion) error {
 	return nil
 }
 
+func (h History) Append(nv ...NameVersion) History {
+	result := make(History, len(h)+len(nv))
+	copy(result, h)
+	copy(result[len(h):], nv)
+	return result
+}
+
 func (h History) Copy() History {
 	return append(h[:0:0], h...)
 }

pkg/contexts/oci/attrs/cacheattr/attr.go

@@ -70,10 +70,8 @@ func (a AttributeType) Decode(data []byte, unmarshaller runtime.Unmarshaler) (in
 		if err == nil {
 			return accessio.NewStaticBlobCache(value)
 		}
-	} else {
-		if err == nil {
-			err = errors.Newf("file path missing")
-		}
+	} else if err == nil {
+		err = errors.Newf("file path missing")
 	}
 	return value, err
 }

pkg/contexts/oci/core/uniform.go

@@ -82,7 +82,7 @@ func (u *UniformRepositorySpec) ComposeRef(art string) string {
 func (u *UniformRepositorySpec) RepositoryRef() string {
 	t := u.Type
 	if t != "" {
-		t = t + "::"
+		t += "::"
 	}
 	if u.Info != "" {
 		return fmt.Sprintf("%s%s", t, u.Info)

pkg/contexts/oci/cpi/flavor_artefact.go

@@ -257,10 +257,8 @@ func AdjustSize(d *artdesc.Descriptor, size int64) error {
 	if size != accessio.BLOB_UNKNOWN_SIZE {
 		if d.Size == accessio.BLOB_UNKNOWN_SIZE {
 			d.Size = size
-		} else {
-			if d.Size != size {
-				return errors.Newf("blob size mismatch %d != %d", size, d.Size)
-			}
+		} else if d.Size != size {
+			return errors.Newf("blob size mismatch %d != %d", size, d.Size)
 		}
 	}
 	return nil

pkg/contexts/oci/cpi/support/flavor_artefact.go

@@ -232,10 +232,8 @@ func AdjustSize(d *artdesc.Descriptor, size int64) error {
 	if size != accessio.BLOB_UNKNOWN_SIZE {
 		if d.Size == accessio.BLOB_UNKNOWN_SIZE {
 			d.Size = size
-		} else {
-			if d.Size != size {
-				return errors.Newf("blob size mismatch %d != %d", size, d.Size)
-			}
+		} else if d.Size != size {
+			return errors.Newf("blob size mismatch %d != %d", size, d.Size)
 		}
 	}
 	return nil

pkg/contexts/oci/ociutils/helm/ignore/rules.go

@@ -173,9 +173,10 @@ func (r *Rules) parseRule(rule string) error {
 		rule = strings.TrimSuffix(rule, "/")
 	}
 
-	if strings.HasPrefix(rule, "/") {
+	switch {
+	case strings.HasPrefix(rule, "/"):
 		// Require path matches the root path.
-		p.match = func(n string, fi os.FileInfo) bool {
+		p.match = func(n string, _ os.FileInfo) bool {
 			rule = strings.TrimPrefix(rule, "/")
 			ok, err := filepath.Match(rule, n)
 			if err != nil {
@@ -184,18 +185,18 @@ func (r *Rules) parseRule(rule string) error {
 			}
 			return ok
 		}
-	} else if strings.Contains(rule, "/") {
+	case strings.Contains(rule, "/"):
 		// require structural match.
-		p.match = func(n string, fi os.FileInfo) bool {
+		p.match = func(n string, _ os.FileInfo) bool {
 			ok, err := filepath.Match(rule, n)
 			if err != nil {
 				log.Printf("Failed to compile %q: %s", rule, err)
 				return false
 			}
 			return ok
 		}
-	} else {
-		p.match = func(n string, fi os.FileInfo) bool {
+	default:
+		p.match = func(n string, _ os.FileInfo) bool {
 			// When there is no slash in the pattern, we evaluate ONLY the
 			// filename.
 			n = filepath.Base(n)

pkg/contexts/oci/repositories/ctf/index/index.go

@@ -135,10 +135,8 @@ func (r *RepositoryIndex) GetTags(repo string) []string {
 		if !strings.HasPrefix(t, "@") {
 			result = append(result, t)
 			digests[a.Digest] = true
-		} else {
-			if !digests[a.Digest] {
-				digests[a.Digest] = false
-			}
+		} else if !digests[a.Digest] {
+			digests[a.Digest] = false
 		}
 	}
 	/* TODO: how to query untagged entries at api level

pkg/contexts/ocm/accessmethods/ociblob/method.go

@@ -156,10 +156,8 @@ func (m *accessMethod) getBlob() (cpi.BlobAccess, error) {
 	}
 	if m.spec.Size == accessio.BLOB_UNKNOWN_SIZE {
 		m.spec.Size = size
-	} else {
-		if size != accessio.BLOB_UNKNOWN_SIZE {
-			return nil, errors.Newf("blob size mismatch %d != %d", size, m.spec.Size)
-		}
+	} else if size != accessio.BLOB_UNKNOWN_SIZE {
+		return nil, errors.Newf("blob size mismatch %d != %d", size, m.spec.Size)
 	}
 	m.blob = accessio.BlobAccessForDataAccess(m.spec.Digest, m.spec.Size, m.spec.MediaType, acc)
 	return m.blob, nil

pkg/contexts/ocm/compdesc/codecs.go

@@ -93,6 +93,6 @@ var DefaultYAMLCodec = CodecWrapper{
 // DefaultJSONCodec implements Codec interface with the json decoder encoder.
 var DefaultJSONLCodec = CodecWrapper{
 	Decoder:       DecoderFunc(json.Unmarshal),
-	StrictDecoder: StrictDecoderFunc(func(data []byte, obj interface{}) error { return json.Unmarshal(data, obj) }),
+	StrictDecoder: StrictDecoderFunc(json.Unmarshal),
 	Encoder:       EncoderFunc(json.Marshal),
 }

pkg/contexts/ocm/core/uniform.go

@@ -59,7 +59,7 @@ func (r *UniformRepositorySpec) CredHost() string {
 func (u *UniformRepositorySpec) String() string {
 	t := u.Type
 	if t != "" && !ocireg.IsKind(t) {
-		t = t + "::"
+		t += "::"
 	}
 	if u.Info != "" {
 		return fmt.Sprintf("%s%s", t, u.Info)

pkg/contexts/ocm/cpi/support/compversaccess.go

@@ -304,7 +304,7 @@ func (c *componentVersionAccessImpl) SetResource(meta *cpi.ResourceMeta, acc com
 
 	cd := c.GetDescriptor()
 	if idx := cd.GetResourceIndex(meta); idx == -1 {
-		cd.Resources = append(c.GetDescriptor().Resources, *res)
+		cd.Resources = append(cd.Resources, *res)
 		cd.Signatures = nil
 	} else {
 		if !cd.Resources[idx].ResourceMeta.HashEqual(&res.ResourceMeta) {
@@ -332,8 +332,7 @@ func (c *componentVersionAccessImpl) SetSource(meta *cpi.SourceMeta, acc compdes
 		Access:     acc,
 	}
 
-	switch res.Version {
-	case "":
+	if res.Version == "" {
 		res.Version = c.GetVersion()
 	}
 

pkg/contexts/ocm/download/handlers/helm/handler.go

@@ -70,7 +70,7 @@ func (h Handler) Download(ctx out.Context, racc cpi.ResourceAccess, path string,
 		return true, "", errors.Newf("no layers found")
 	}
 	if !strings.HasSuffix(path, ".tgz") {
-		path = path + ".tgz"
+		path += ".tgz"
 	}
 	blob, err := m.GetBlob(m.GetDescriptor().Layers[0].Digest)
 	if err != nil {

pkg/contexts/ocm/ref.go

@@ -133,7 +133,7 @@ func (r *RefSpec) HostPort() (string, string) {
 func (r *RefSpec) Reference() string {
 	t := r.Type
 	if t != "" {
-		t = t + "::"
+		t += "::"
 	}
 	s := r.SubPath
 	if s != "" {