Repository: kubernetes-sigs/krew
Pull request #370 (2019-11-09 12:27:06)

Rely on standard utils for `IsSubPath`
* Add more test cases
* Fix misleading argument naming of `IsSubPath`
       - subPath -> basePath
       - path    -> subPath

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/370#" title="Author self-approved">corneliusweig</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [corneliusweig]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov-io: # [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/370?src=pr&el=h1) Report
> Merging [#370](https://codecov.io/gh/kubernetes-sigs/krew/pull/370?src=pr&el=desc) into [master](https://codecov.io/gh/kubernetes-sigs/krew/commit/8f520117c8c2effa939d9a53fd3106a9f45c8630?src=pr&el=desc) will **decrease** coverage by `0.09%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/kubernetes-sigs/krew/pull/370/graphs/tree.svg?width=650&token=J3nFtARI2m&height=150&src=pr)](https://codecov.io/gh/kubernetes-sigs/krew/pull/370?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master     #370     +/-   ##
=========================================
- Coverage    57.2%   57.11%   -0.1%     
=========================================
  Files          19       19             
  Lines         916      914      -2     
=========================================
- Hits          524      522      -2     
  Misses        339      339             
  Partials       53       53
```


| [Impacted Files](https://codecov.io/gh/kubernetes-sigs/krew/pull/370?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/pathutil/pathutil.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/370/diff?src=pr&el=tree#diff-cGtnL3BhdGh1dGlsL3BhdGh1dGlsLmdv) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/370?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/370?src=pr&el=footer). Last update [8f52011...4d52558](https://codecov.io/gh/kubernetes-sigs/krew/pull/370?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/hack/.gitignore b/hack/.gitignore
new file mode 100644
index 0000000..4724abf
--- /dev/null
+++ b/hack/.gitignore
@@ -0,0 +1,1 @@
+/release-notes
diff --git a/hack/make-release-notes.sh b/hack/make-release-notes.sh
index 78cef9f..6989ca7 100755
--- a/hack/make-release-notes.sh
+++ b/hack/make-release-notes.sh
@@ -18,6 +18,10 @@
 # You can run this script while tagging the release as:
 #     git tag -a v0.1 -m "$(TAG=v0.1 hack/make-release-notes.sh)"
 
+set -euo pipefail
+
+SCRIPTDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
 TAG="${TAG:?TAG environment variable must be set for this script}"
 if ! [[ "$TAG" =~ v.* ]]; then
   echo >&2 "TAG must be in format v.*"
@@ -34,6 +38,22 @@ download_assets=(
   krew.yaml
 )
 
+install_release_notes() {
+  relnotes_dir="$(mktemp -d)"
+  trap 'rm -rf -- ${relnotes_dir}' EXIT
+
+  cd "$relnotes_dir"
+  go mod init foo
+  GOBIN="${SCRIPTDIR}" go install github.com/corneliusweig/release-notes
+  cd -
+}
+
+# install release-notes if not present in the hack folder
+if ! [[ -x "${SCRIPTDIR}/release-notes" ]]; then
+  echo >&2 'Installing release-notes'
+  install_release_notes
+fi
+
 echo "Installation"
 echo "------------"
 echo "To install this release, refer to the instructions at ${readme}."
@@ -54,3 +74,11 @@ git log "$(git describe --tags --abbrev=0)..HEAD" --format=%an |
   sed -E 's,^(\s+[0-9]+\s),- ,g'
 echo
 echo "(krew ${TAG} is tagged on $(date -u).)"
+echo
+echo '<details>'
+echo '<summary>Merged pull requests</summary>'
+echo # this empty line is important for correct markdown rendering
+# you can pass your github token with --token here if you run out of requests
+"${SCRIPTDIR}/release-notes" kubernetes-sigs krew
+echo '</details>'
+echo
diff --git a/pkg/pathutil/pathutil.go b/pkg/pathutil/pathutil.go
index e30d602..b672426 100644
--- a/pkg/pathutil/pathutil.go
+++ b/pkg/pathutil/pathutil.go
@@ -23,23 +23,15 @@ import (
 
 // IsSubPath checks if the extending path is an extension of the basePath, it will return the extending path
 // elements. Both paths have to be absolute or have the same root directory. The remaining path elements
-func IsSubPath(subPath, path string) ([]string, bool) {
-	basePieces := strings.Split(filepath.Clean(subPath), string(filepath.Separator))
-	extendingPieces := strings.Split(filepath.Clean(path), string(filepath.Separator))
-
-	// the binary has to be in the install path.
-	if len(basePieces) > len(extendingPieces) {
-		return nil, false
+func IsSubPath(basePath, subPath string) (string, bool) {
+	extendingPath, err := filepath.Rel(basePath, subPath)
+	if err != nil {
+		return "", false
 	}
-
-	// Compare path pieces.
-	for i, p := range basePieces {
-		if extendingPieces[i] != p {
-			return nil, false
-		}
+	if strings.HasPrefix(extendingPath, "..") {
+		return "", false
 	}
-
-	return extendingPieces[len(basePieces):], true
+	return extendingPath, true
 }
 
 // ReplaceBase will return a replacement path with replacement as a base of the path instead of the old base. a/b/c, a, d -> d/b/c
@@ -48,5 +40,5 @@ func ReplaceBase(path, old, replacement string) (string, error) {
 	if !ok {
 		return "", errors.Errorf("can't replace %q in %q, it is not a subpath", old, path)
 	}
-	return filepath.Join(replacement, filepath.Join(extendingPath...)), nil
+	return filepath.Join(replacement, extendingPath), nil
 }


Test patch:
diff --git a/pkg/pathutil/pathutil_test.go b/pkg/pathutil/pathutil_test.go
index a968624..10c350c 100644
--- a/pkg/pathutil/pathutil_test.go
+++ b/pkg/pathutil/pathutil_test.go
@@ -28,44 +28,81 @@ func TestIsSubPathExtending(t *testing.T) {
 	tests := []struct {
 		name            string
 		args            args
-		wantExtending   []string
+		wantExtending   string
 		wantIsExtending bool
 	}{
 		{
 			name: "is extending",
 			args: args{
-				basePath:      filepath.Join("a", "b"),
-				extendingPath: filepath.Join("a", "b", "c"),
+				basePath:      filepath.FromSlash("a/b"),
+				extendingPath: filepath.FromSlash("a/b/c"),
 			},
-			wantExtending:   []string{"c"},
+			wantExtending:   "c",
 			wantIsExtending: true,
 		},
 		{
 			name: "is extending same length",
 			args: args{
-				basePath:      filepath.Join("a", "b", "c"),
-				extendingPath: filepath.Join("a", "b", "c"),
+				basePath:      filepath.FromSlash("a/b/c"),
+				extendingPath: filepath.FromSlash("a/b/c"),
 			},
-			wantExtending:   []string{},
+			wantExtending:   ".",
 			wantIsExtending: true,
 		},
 		{
 			name: "is not extending",
 			args: args{
-				basePath:      filepath.Join("a", "b"),
-				extendingPath: filepath.Join("a", "a", "c"),
+				basePath:      filepath.FromSlash("a/b"),
+				extendingPath: filepath.FromSlash("a/a/c"),
 			},
-			wantExtending:   nil,
-			wantIsExtending: false,
 		},
 		{
 			name: "is not smaller extending",
 			args: args{
-				basePath:      filepath.Join("a", "b"),
-				extendingPath: filepath.Join("a"),
+				basePath:      filepath.FromSlash("a/b"),
+				extendingPath: filepath.FromSlash("a"),
 			},
-			wantExtending:   nil,
-			wantIsExtending: false,
+		},
+		{
+			name: "base path is not clean",
+			args: args{
+				basePath:      filepath.FromSlash("d/../a/b/../../a"),
+				extendingPath: filepath.FromSlash("a/b"),
+			},
+			wantExtending:   "b",
+			wantIsExtending: true,
+		},
+		{
+			name: "extending path is not clean",
+			args: args{
+				basePath:      filepath.FromSlash("a"),
+				extendingPath: filepath.FromSlash("d/../a/b/c/.."),
+			},
+			wantExtending:   "b",
+			wantIsExtending: true,
+		},
+		{
+			name: "base path is absolute",
+			args: args{
+				basePath:      filepath.FromSlash("/a"),
+				extendingPath: filepath.FromSlash("a/b"),
+			},
+		},
+		{
+			name: "extending path is absolute",
+			args: args{
+				basePath:      filepath.FromSlash("a"),
+				extendingPath: filepath.FromSlash("/a/b"),
+			},
+		},
+		{
+			name: "both paths are absolute",
+			args: args{
+				basePath:      filepath.FromSlash("/a"),
+				extendingPath: filepath.FromSlash("/a/b"),
+			},
+			wantExtending:   "b",
+			wantIsExtending: true,
 		},
 	}
 	for _, tt := range tests {