Refactor and fix find bugs #2273

harshavardhana · 2017-10-06T03:47:04Z

Watch now avoids an unnecessary waitgroup and simplifies overall flow.
Watch now honors maxDepth parameter as well.
Parsing logic is fully outside in the mainFind() function.
Enhance and fix behavior of nameMatch to comply with unix
find behavior.
Enhance and fix behavior of maxdepth to comply with unix
find behavior.

harshavardhana · 2017-10-06T03:47:22Z

Still working on the unit tests, but it is ready for the early eyes.

codecov-io · 2017-10-06T06:11:59Z

Codecov Report

Merging #2273 into master will increase coverage by 0.43%.
The diff coverage is 21.62%.

@@            Coverage Diff             @@
##           master    #2273      +/-   ##
==========================================
+ Coverage   10.12%   10.55%   +0.43%     
==========================================
  Files         100      100              
  Lines        9582     9598      +16     
==========================================
+ Hits          970     1013      +43     
+ Misses       8477     8450      -27     
  Partials      135      135

Impacted Files	Coverage Δ
cmd/watch.go	`0% <ø> (ø)`	⬆️
cmd/watch-main.go	`0% <ø> (ø)`	⬆️
cmd/find-main.go	`0% <0%> (ø)`	⬆️
cmd/find.go	`44.15% <31.25%> (+18.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f38471...6c0bec5. Read the comment docs.

harshavardhana · 2017-10-06T07:50:48Z

Still working on the unit tests, but it is ready for the early eyes.

Done, ready for review.

kannappanr · 2017-10-06T22:29:52Z

cmd/find-main.go

 	for _, arg := range args {
 		if strings.TrimSpace(arg) == "" {
 			fatalIf(errInvalidArgument().Trace(args...), "Unable to validate empty argument.")
 		}
 	}

+	// Extract input URLs.
+	URLs := ctx.Args()
+	for _, url := range URLs {


comment like "Validate the extracted URLs. " will be good.

kannappanr · 2017-10-06T22:46:51Z

cmd/find-main.go

+		fatalIf(err.Trace(ctx.String("newer")), "Unable to parse input threshold value into time.Time")
+	}
+
+	var e error


Why can't we use already declared 'err' variable?

they both have different type.

kannappanr · 2017-10-06T23:08:25Z

cmd/find.go

@@ -74,6 +73,14 @@ func (f findMessage) JSON() string {
 func nameMatch(pattern, path string) bool {
 	matched, e := filepath.Match(pattern, filepath.Base(path))
 	errorIf(probe.NewError(e).Trace(pattern, path), "Unable to match with input pattern")
+	if !matched {


When will this happen? Meaning do you have an example for me to better understand when this piece of code will be needed.

It is a compatibility code.. it will happen when the filepath.Match() didn't match say for example if user said mc find pkg --name console

kannappanr · 2017-10-06T23:53:54Z

cmd/find.go

@@ -145,30 +115,95 @@ func doFindExec(ctx *cli.Context, fileContent contentMessage) {
 	console.Println(string(out.Bytes()))
 }

-// doFindWatch - enables listening on the input path, listens for all file/object
+// watchFind - enables listening on the input path, listens for all file/object


Line 113 above, looks like unreachable code. If it is unreachable, we need to remove it.

ebozduman · 2017-10-07T00:50:39Z

cmd/find-main.go

@@ -67,17 +70,13 @@ var (
 			Name:  "smaller",
 			Usage: "Match all objects smaller than specified size in units (see UNITS)",
 		},
-		cli.StringFlag{
+		cli.UintFlag{


Can we have the flags alphabetically sorted in the usage below, line#97, for readability purposes, and here for debugging purposes?

kannappanr

LGTM

ebozduman · 2017-10-07T01:17:41Z

cmd/find-main.go

+	)
+	if ctx.String("older") != "" {
+		olderThan, err = parseTime(ctx.String("older"))
+		fatalIf(err.Trace(ctx.String("older")), "Unable to parse input threshold value into time.Time")


time.Time in the error message not necessarily means something useful to the end-user.
We need to have our messages in the simplest possible way without assuming anything about the end-user's programming knowledge and/or background.
This also applies to line#232.

ebozduman · 2017-10-09T18:53:12Z

cmd/find-main.go

-	if ctx.Bool("watch") && !strings.Contains(args[0], "/") {
-		console.Println("Users must specify a bucket name for watch")
-		console.Fatalln()
+	if !ctx.Args().Present() {


Is there any specific reason you preferred to use ctx.Args instead of args?

ebozduman · 2017-10-09T18:58:36Z

cmd/find-main.go

+	if !ctx.Args().Present() {
+		args = []string{"./"} // No args just default to present directory.
+	} else if ctx.Args().Get(0) == "." {
+		ctx.Args()[0] = "./" // If the arg is '.' treat it as './'.


Did you mean args[0] = "./" here or is this for a reason?
If so, I guess we should use args all over after assigning args := ctx.Args() is done.

ebozduman · 2017-10-09T20:57:56Z

cmd/find-main.go

 	for _, arg := range args {
 		if strings.TrimSpace(arg) == "" {
 			fatalIf(errInvalidArgument().Trace(args...), "Unable to validate empty argument.")
 		}
 	}

+	// Extract input URLs and validate.
+	URLs := ctx.Args()
+	for _, url := range URLs {


Why not have for _, url := range args { and get rid of line#178 completely?
Also change the comment at line#177 to // Extract input urls and validate.

ebozduman · 2017-10-09T22:10:29Z

cmd/find-main.go

-
-	if !ctx.Args().Present() {
-		args = []string{"."}
+	if !args.Present() {


Checks and codes between line#222 and line#227 are already covered as part of the call to checkFindSyntax(ctx). So, they are unnecessary and can be removed.

This is needed for argument adjustment.

ebozduman · 2017-10-09T22:20:46Z

cmd/find-main.go

-	fatalIf(err.Trace(args...), "Unable to initialize `"+args[0]+"`")
+	fatalIf(err.Trace(args...), "Unable to initialize `"+args[0]+"`.")
+
+	var (


It could be declared like var olderThan, newerThan time.Time

ebozduman · 2017-10-09T22:29:47Z

cmd/find.go

+		return path
+	}
+	// Remove the requested prefix from consideration, maxDepth is
+	// only considered for all other levels excluding th starting prefix.


...excluding th? starting prefix. ?

ebozduman · 2017-10-09T22:58:16Z

cmd/find.go

-		match = smallerSizeMatch(ctx.String("smaller"), fileContent.Size)
+	if ctx.ignorePattern != "" {
+		match = !pathMatch(ctx.ignorePattern, fileContent.Key)
+	} else if ctx.namePattern != "" {


To be able to combine find options/choices with AND, these elses need to be removed.

- Watch now avoids an unnecessary waitgroup and simplifies overall flow. - Watch now honors maxDepth parameter as well. - Parsing logic is fully outside in the mainFind() function. - Enhance and fix behavior of nameMatch to comply with unix find behavior. - Enhance and fix behavior of maxdepth to comply with unix find behavior. Fixes minio#2272

kannappanr · 2017-10-09T23:29:06Z

cmd/find.go

-		match = largerSizeMatch(ctx.String("larger"), fileContent.Size)
-	} else if ctx.String("smaller") != "" {
-		match = smallerSizeMatch(ctx.String("smaller"), fileContent.Size)
+	if match && ctx.ignorePattern != "" {


match here is unncessary

It is for completeness.. otherwise logically it is hard.

ebozduman

LGTM

harshavardhana force-pushed the find-cleanup branch from f065c8f to 36feb55 Compare October 6, 2017 06:08

harshavardhana force-pushed the find-cleanup branch from 36feb55 to 11c7faa Compare October 6, 2017 07:48

harshavardhana changed the title ~~Fixes find command and bring it to a workable condition.~~ Refactor and fix find bugs Oct 6, 2017

harshavardhana force-pushed the find-cleanup branch from 11c7faa to 46a6dff Compare October 6, 2017 07:50

harshavardhana force-pushed the find-cleanup branch from 46a6dff to 4e9c547 Compare October 6, 2017 07:57

deekoder requested review from ebozduman and kannappanr October 6, 2017 16:35

harshavardhana force-pushed the find-cleanup branch from 4e9c547 to bfbd832 Compare October 6, 2017 18:26

kannappanr reviewed Oct 6, 2017

View reviewed changes

harshavardhana force-pushed the find-cleanup branch from bfbd832 to 1c860b1 Compare October 6, 2017 22:55

kannappanr reviewed Oct 6, 2017

View reviewed changes

ebozduman reviewed Oct 7, 2017

View reviewed changes

harshavardhana force-pushed the find-cleanup branch 3 times, most recently from 4099e2e to e1ed1fc Compare October 7, 2017 02:00

kannappanr approved these changes Oct 7, 2017

View reviewed changes

harshavardhana force-pushed the find-cleanup branch 3 times, most recently from 02dffe5 to 9a436e4 Compare October 9, 2017 07:01

ebozduman requested changes Oct 9, 2017

View reviewed changes

harshavardhana force-pushed the find-cleanup branch from 9a436e4 to 75fd5e4 Compare October 9, 2017 19:27

ebozduman requested changes Oct 9, 2017

View reviewed changes

harshavardhana force-pushed the find-cleanup branch from 75fd5e4 to a8631aa Compare October 9, 2017 22:58

harshavardhana force-pushed the find-cleanup branch from a8631aa to 6c0bec5 Compare October 9, 2017 23:11

kannappanr reviewed Oct 9, 2017

View reviewed changes

ebozduman approved these changes Oct 9, 2017

View reviewed changes

harshavardhana merged commit c10b429 into minio:master Oct 10, 2017

harshavardhana deleted the find-cleanup branch October 10, 2017 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and fix find bugs #2273

Refactor and fix find bugs #2273

harshavardhana commented Oct 6, 2017 •

edited

Loading

harshavardhana commented Oct 6, 2017

codecov-io commented Oct 6, 2017 •

edited

Loading

harshavardhana commented Oct 6, 2017

kannappanr Oct 6, 2017

harshavardhana Oct 6, 2017

kannappanr Oct 6, 2017

harshavardhana Oct 6, 2017

kannappanr Oct 6, 2017 •

edited

Loading

harshavardhana Oct 7, 2017

kannappanr Oct 6, 2017

ebozduman Oct 7, 2017 •

edited

Loading

kannappanr left a comment

ebozduman Oct 7, 2017

ebozduman Oct 9, 2017

harshavardhana Oct 9, 2017

ebozduman Oct 9, 2017

harshavardhana Oct 9, 2017

ebozduman Oct 9, 2017

harshavardhana Oct 9, 2017

ebozduman Oct 9, 2017

harshavardhana Oct 9, 2017

ebozduman Oct 9, 2017

ebozduman Oct 9, 2017

ebozduman Oct 9, 2017 •

edited

Loading

kannappanr Oct 9, 2017

harshavardhana Oct 9, 2017

kannappanr Oct 10, 2017

ebozduman left a comment

Refactor and fix find bugs #2273

Refactor and fix find bugs #2273

Conversation

harshavardhana commented Oct 6, 2017 • edited Loading

harshavardhana commented Oct 6, 2017

codecov-io commented Oct 6, 2017 • edited Loading

Codecov Report

harshavardhana commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannappanr Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebozduman Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

kannappanr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebozduman Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebozduman left a comment

Choose a reason for hiding this comment

harshavardhana commented Oct 6, 2017 •

edited

Loading

codecov-io commented Oct 6, 2017 •

edited

Loading

kannappanr Oct 6, 2017 •

edited

Loading

ebozduman Oct 7, 2017 •

edited

Loading

ebozduman Oct 9, 2017 •

edited

Loading