code review 2476041: path: add Glob

path: add Glob

As discussed in http://groups.google.com/group/golang-dev/browse_thread/thread/926b7d550d98ec9e,
add a simple "path expander" function, which returns all the
files matching the given pattern. This function is called Glob
after glob(3) in libc.

Also add a convenience function, hasMeta, that checks whether
a string contains one of the characters which are specially handled
by Match.

[bsiegert, 2010-10-14 19:11:29]

Hello rsc, golang-dev@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[bsiegert, 2010-10-22 07:56:12]

Ping?

On Thu, Oct 14, 2010 at 21:11,  <bsiegert@gmail.com> wrote:
> Reviewers: rsc, golang-dev_golang.org,
>
> Message:
> Hello rsc, golang-dev@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> Add path.Glob and path.ContainsMagic
>
> As discussed in
>
http://groups.google.com/group/golang-dev/browse_thread/thread/926b7d550d98ec9e,
> add a simple "path expander" function, which returns all the
> files matching the given pattern. This function is called Glob
> after glob(3) in libc.
>
> Also add a convenience function, ContainsMagic, that checks whether
> a string contains one of the characters which are specially handled
> by Match.
>
> Please review this at http://codereview.appspot.com/2476041/
>
> Affected files:
>  M src/pkg/path/match.go
>
>
> Index: src/pkg/path/match.go
> ===================================================================
> --- a/src/pkg/path/match.go
> +++ b/src/pkg/path/match.go
> @@ -1,6 +1,7 @@
>  package path
>
>  import (
> +       "container/vector"
>        "os"
>        "strings"
>        "utf8"
> @@ -74,6 +75,70 @@
>        return len(name) == 0, nil
>  }
>
> +// Glob returns the names of all files matching pattern or nil
> +// if there is no matching file. The syntax used by pattern is like in
> +// Match. The pattern can contain a pathname.
> +//
> +func Glob(pattern string) []string {
> +       var m, matches vector.StringVector
> +
> +       if !ContainsMagic(pattern) {
> +               if _, err := os.Stat(pattern); err == nil {
> +                       return []string{pattern}
> +               }
> +               return nil
> +       }
> +
> +       dir, file := Split(pattern)
> +       switch dir {
> +       case "":
> +               dir = "."
> +       case "/":
> +               // nothing
> +       default:
> +               dir = dir[0 : len(dir)-1] // chop off tailing '/'
> +       }
> +
> +       if ContainsMagic(dir) {
> +               for _, d := range Glob(dir) {
> +                       m = Glob(Join(d, file))
> +                       matches.AppendVector(&m)
> +               }
> +       } else {
> +               fi, err := os.Stat(dir)
> +               if err != nil || !fi.IsDirectory() {
> +                       return nil
> +               }
> +               d, err := os.Open(dir, os.O_RDONLY, 0666)
> +               if err != nil {
> +                       return nil
> +               }
> +               defer d.Close()
> +
> +               names, err := d.Readdirnames(-1)
> +               if err != nil {
> +                       return nil
> +               }
> +
> +               for _, n := range names {
> +                       didmatch, err := Match(file, n)
> +                       if err != nil {
> +                               return nil
> +                       }
> +                       if didmatch {
> +                               matches.Push(Join(dir, n))
> +                       }
> +               }
> +       }
> +       return matches
> +}
> +
> +// ContainsMagic returns true if path contains any of the magic characters
> +// recognized by Match.
> +func ContainsMagic(path string) bool {
> +       return strings.IndexAny(path, "*?[") != -1
> +}
> +
>  // scanChunk gets the next section of pattern, which is a non-star string
>  // possibly preceded by a star.
>  func scanChunk(pattern string) (star bool, chunk, rest string) {
>
>
>

[rsc, 2010-10-22 13:56:42]

This is pretty nice code.  Thanks.

ContainsMagic isn't the right name.
Maybe HasWildcard, or better, hasWildcard.
(I don't believe it needs to be exported.)

The recursive implementation of Glob is
elegant but won't work in general.
Try this:

mkdir '*'
touch '*/foo'

and then path.Glob("*/foo")

Russ

[r2, 2010-10-22 14:29:02]

On Oct 22, 2010, at 6:56 AM, Russ Cox wrote:

> This is pretty nice code.  Thanks.
> 
> ContainsMagic isn't the right name.
> Maybe HasWildcard, or better, hasWildcard.
> (I don't believe it needs to be exported.)

The regexp package calls these Meta.  How about HasMeta?

> The recursive implementation of Glob is
> elegant but won't work in general.
> Try this:
> 
> mkdir '*'
> touch '*/foo'
> 
> and then path.Glob("*/foo")
> 
> Russ

[bsiegert, 2010-10-26 19:24:39]

Hello rsc, golang-dev@golang.org, r2 (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-11-01 20:36:45]

please add tests

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go
File src/pkg/path/match.go (right):

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcode4
src/pkg/path/match.go:4: "container/vector"
should be able to use append instead of importing vector

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcode99
src/pkg/path/match.go:99: dir = dir[0 : len(dir)-1] // chop off tailing '/'
trailing

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:104: m = globInDir(d, file)
matches = glob(d, file, matches)

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:114: func globInDir(dir, pattern string) []string {
s/globInDir/glob/

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:127: names, err := d.Readdirnames(-1)
should be iotuil.ReadDir so that you get the names in sorted order

[bsiegert, 2010-11-04 20:11:23]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[bsiegert, 2010-11-04 20:13:43]

Also added tests, as requested.

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go
File src/pkg/path/match.go (right):

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcode4
src/pkg/path/match.go:4: "container/vector"
On 2010/11/01 20:36:46, rsc1 wrote:
> should be able to use append instead of importing vector

Done.

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcode99
src/pkg/path/match.go:99: dir = dir[0 : len(dir)-1] // chop off tailing '/'
On 2010/11/01 20:36:46, rsc1 wrote:
> trailing

Done.

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:104: m = globInDir(d, file)
On 2010/11/01 20:36:46, rsc1 wrote:
> matches = glob(d, file, matches)

Done. Good idea.

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:114: func globInDir(dir, pattern string) []string {
On 2010/11/01 20:36:46, rsc1 wrote:
> s/globInDir/glob/

Done.

http://codereview.appspot.com/2476041/diff/12001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:127: names, err := d.Readdirnames(-1)
On 2010/11/01 20:36:46, rsc1 wrote:
> should be iotuil.ReadDir so that you get the names in sorted order

Isn't this much more expensive because ioutil.ReadDir returns []*os.FileInfo, of
which we only need the names? Anyway, done.

[rsc, 2010-11-04 20:22:21]

> On 2010/11/01 20:36:46, rsc1 wrote:
>>
>> should be iotuil.ReadDir so that you get the names in sorted order
>
> Isn't this much more expensive because ioutil.ReadDir returns
> []*os.FileInfo, of which we only need the names? Anyway, done.

indeed, whoops.  it would be fine and probably better to go back
to Readdirnames and then sort.SortStrings(names) before doing
the walk.

i suggested it because i saw that path.Walk was already using it,
but path.Walk really wants the *os.FileInfo; Glob doesn't.

nice catch.

russ

[bsiegert, 2010-11-04 20:36:39]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2010-11-04 20:50:05]

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go
File src/pkg/path/match.go (right):

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcode78
src/pkg/path/match.go:78: // Glob returns the names of all files matching
pattern or nil
these routines should be at the end of the file, after the implementation and
subfunctions of Match

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcode79
src/pkg/path/match.go:79: // if there is no matching file. The syntax used by
pattern is like in
s/like in/the same as in/

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcode80
src/pkg/path/match.go:80: // Match. The pattern can contain a pathname.
pathname is not a word.  in any case i don't understand what this sentence
means.

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:145: func hasMeta(path string) bool {
a case could be made that this function be exported

[bsiegert, 2010-11-04 21:21:41]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[bsiegert, 2010-11-04 21:26:25]

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go
File src/pkg/path/match.go (right):

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcode78
src/pkg/path/match.go:78: // Glob returns the names of all files matching
pattern or nil
On 2010/11/04 20:50:05, r wrote:
> these routines should be at the end of the file, after the implementation and
> subfunctions of Match

Done.

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcode79
src/pkg/path/match.go:79: // if there is no matching file. The syntax used by
pattern is like in
On 2010/11/04 20:50:05, r wrote:
> s/like in/the same as in/

Done.

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcode80
src/pkg/path/match.go:80: // Match. The pattern can contain a pathname.
On 2010/11/04 20:50:05, r wrote:
> pathname is not a word.  in any case i don't understand what this sentence
> means.

s/pathname/path/. It means that Glob accepts full paths as patterns, not only
filenames. Do you have an idea for an improved wording?

http://codereview.appspot.com/2476041/diff/27001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:145: func hasMeta(path string) bool {
On 2010/11/04 20:50:05, r wrote:
> a case could be made that this function be exported

rsc prodded me to make it non-exported a few revisions ago. Export or not?

[r, 2010-11-04 22:40:34]

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go
File src/pkg/path/match.go (right):

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:208: // if there is no matching file. The syntax used by
pattern is the same
s/used by pattern/of patterns/

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:209: // as in Match. The pattern can contain a path.
it's still not clear what it means to say 'the pattern can contain a path'. 
"foo" is a path.

how about

Unlike in Glob, the pattern may describe hierarchical names such as
/usr/*/bin/ed.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:211: func Glob(pattern string) []string {
s/[]string/(matches []string)

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:212: var matches []string
delete

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:236: return glob(dir, file, []string{})
the last argument can be nil.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:245: if err != nil || !fi.IsDirectory() {
merge these two lines. fi isn't used below.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:261: didmatch, err := Match(pattern, n)
s/didmatch/matched/

[bsiegert, 2010-11-05 11:35:06]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[bsiegert, 2010-11-05 11:36:20]

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go
File src/pkg/path/match.go (right):

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:208: // if there is no matching file. The syntax used by
pattern is the same
On 2010/11/04 22:40:34, r wrote:
> s/used by pattern/of patterns/

Done. Cf. "the syntax used by pattern" in the description of Match.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:209: // as in Match. The pattern can contain a path.
On 2010/11/04 22:40:34, r wrote:
> it's still not clear what it means to say 'the pattern can contain a path'. 
> "foo" is a path.
> 
> how about
> 
> Unlike in Glob, the pattern may describe hierarchical names such as
> /usr/*/bin/ed.

Done.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:211: func Glob(pattern string) []string {
On 2010/11/04 22:40:34, r wrote:
> s/[]string/(matches []string)

Done.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:212: var matches []string
On 2010/11/04 22:40:34, r wrote:
> delete

Done.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:236: return glob(dir, file, []string{})
On 2010/11/04 22:40:34, r wrote:
> the last argument can be nil.

Done.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:245: if err != nil || !fi.IsDirectory() {
On 2010/11/04 22:40:34, r wrote:
> merge these two lines. fi isn't used below.

Done.

http://codereview.appspot.com/2476041/diff/37001/src/pkg/path/match.go#newcod...
src/pkg/path/match.go:261: didmatch, err := Match(pattern, n)
On 2010/11/04 22:40:34, r wrote:
> s/didmatch/matched/

Done.

[rsc1, 2010-11-05 16:06:34]

LGTM

leaving for r

[r, 2010-11-05 17:37:02]

LGTM

[r2, 2010-11-05 17:40:28]

LGTM

-rob

[r, 2010-11-05 17:48:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=1191d7568243 ***

path: add Glob

As discussed in
http://groups.google.com/group/golang-dev/browse_thread/thread/926b7d550d98ec9e,
add a simple "path expander" function, which returns all the
files matching the given pattern. This function is called Glob
after glob(3) in libc.

Also add a convenience function, hasMeta, that checks whether
a string contains one of the characters which are specially handled
by Match.

R=rsc, r, r2
CC=golang-dev
http://codereview.appspot.com/2476041

Committer: Rob Pike <r@golang.org>