Recursive globs behaviour has changed

[nhooyr]

See #232 (comment)

This seems to have broken recursive globs like the following:
https://github.com/cdr/code-server/blob/1826399cd230acc1f62556a5af7bba9c25a03a25/ci/build/nfpm.yaml#L19

./release-standalone/**/*: "/usr/lib/code-server/"

Only files two levels below the top level are copied now whereas before all files were.
Caused a regression in code-server as nfpm wouldn't include the files in ./release-standalone/* but only ./release-standalone/*/**/*. i.e only files in subdirectories of the top level.
See coder/code-server#2310

[caarlos0]

The previous behavior was "wrong"... you can try it with ls. I think what you want is ./release-standalone/**.

**/* implies only files within subdirectories.

I will make this change clearer on the release notes though.

[erikgeiser]

Hm, I thought so too at first (that **/* implies only files with subdirectories). However, I just tested it with three files that contain their own file name. This way I can demonstrate it with cat which does does do special directory processing like ls:

$ tree
.
├── a
│   └── b
│       └── f1
├── c
│   └── f2
└── f3

3 directories, 3 files

$ cat ./**/*
cat: ./a: Is a directory
cat: ./a/b: Is a directory
f1
cat: ./c: Is a directory
f2
f3

$ echo $SHELL
/bin/zsh

$ /bin/zsh --version
zsh 5.7.1 (x86_64-apple-darwin19.0)

So could indeed be a bug in https://github.com/gobwas/glob that we mistook for a feature.

[erikgeiser]

They probably would match ** to an empty string but don't collapse multiple separator characters so ./**/* would match .//foo which we consider equivalent to ./foo.

[erikgeiser]

This indeed seems to be what's happening: https://play.golang.org/p/geU9afuijny

There is also a closed issue for this (gobwas/glob#20). It seems like gobwas thinks that the current behaviour is correct. I made a new issue though: gobwas/glob#46

[caarlos0]

yeah, makes sense @erikgeiser 🤔

[caarlos0]

Maybe we could hack around it and replace **/* with ** only? 🤔
No idea about the side effects of this though

[nhooyr]

Ah, I thought I tried ** at some point and nfpm didn't see any files but it must have been PEBCAK.

I'll switch to that and upgrade us back.

[caarlos0]

PEBCAK

TIL: PEBCAK 🤣

[erikgeiser]

I'm not sure any solution is right. There seem to be so many glob variants that behave differently in these exact cases. This behavior is also configurable in many shells. We probably just have to decide on a behavior we like.

Just hacking around on the pattern before compiling does not seem like a good long term solution through.

[caarlos0]

Maybe forking is a good workaround for now?

[erikgeiser]

First we need to decide what we'll do exactly. We can't really rely on shell behaviour because shell globbing behaviour is also configurable in most shells.

My zsh for example also treats bare ** at the end differently than we do now:

$ tree
.
├── a
│   └── b
│       └── f1
├── c
│   └── f2
└── f3

3 directories, 3 files
$ cat ./**
cat: ./a: Is a directory
cat: ./c: Is a directory
f3

To fix this issue here (/**/*) we could probably replace /**/ in the AST with {/**/,/}. However, we will have to decide how we move forward with this if gobwas/glob does not want these changes. Do we continue our fork?

[caarlos0]

Do we continue our fork?

I would say if they don't want the changes back, either that or leave as-is. So far only this issue was opened about this behavior change, so I would say its probably not too bad to leave as-is....