Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document the unusual behaviour of ** in glob option #941

Conversation

jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Jan 30, 2025

⚡ Summary

I had assumed that glob patterns work like they do with the *nix ls command, or in tools like lint-staged. I can imagine people expecting it to behave the same.

It seems that ** is interpreted as a single asterisk, ie meaning "1+ directories deep", rather than "0 or more directories deep".

Whilst there is a link to the Go glob library used, and that README mentions double asterisks, I was mainly left confused by it:

Syntax is inspired by standard wildcards, except that ** is aka super-asterisk, that do not sensitive for separators.

I don't think this covers the difference I'm bringing up. I also don't know what is meant by "super-asterisk" here - I'd love to know!

More details, including an example are in the commit message.

Thanks for making such an awesome tool!

@jamiecobbett
Copy link
Contributor Author

jamiecobbett commented Jan 30, 2025

I've also noticed that if you use the root: option, then it seems the paths in glob: don't take that into account - ie you must specify the full path, whereas I assumed it would be relative to the root.

If that's correct, I'm happy to add an explanation to the docs that to this PR, or open a separate one.

I had assumed that glob patterns work like they do with the *nix `ls` command,
or in tools like lint-staged.

It seems that ** is interpreted as a single asterisk for 1+ directories, rather than "0 or more directories deep" as in `ls`.

Whilst there is a link to the Go glob library used, and it mentions double
asterisks, I was mainly left confused by the line in the README:

> Syntax is inspired by [standard
> wildcards](http://tldp.org/LDP/GNU-Linux-Tools-Summary/html/x11655.htm),
> except that ** is aka super-asterisk, that do not sensitive for separators.

Whilst the glob library README could be improved, I think it's natural to use
`ls` to try and test what would be matched (or copy config from tools like
lint-staged or pre-commit), so it would be great to call out this difference in
the lefthook documentation.

It might be quite useful to provide a means of testing/listing what files would
be matched for different commands - I found it necessary to do the below.

Example
=======

Let's say you have a directory structure like:

```
- src
  - foo
    - bar
      - nestedTwice.js
    - nestedOnce.js
  - topLevel.js
```

If you run `ls -1`, you get all three files:
```
ls -1 src/**/*.js
src/foo/bar/nestedTwice.js
src/foo/nestedOnce.js
src/topLevel.js
```

Given this lefthook config

```yaml
pre-commit:
  commands:
    eslint:
      run: ls -1 {staged_files}
      glob: "src/**/*.js"
```

And you run:

```sh
git add src
lefthook run pre-commit
```

The output is missing `src/topLevel.js`:

```
src/foo/bar/nestedTwice.js
src/foo/nestedOnce.js
```
@jamiecobbett jamiecobbett force-pushed the add-notes-about-double-asterisk-glob-behaviour branch from 25c058b to 5fde6d6 Compare February 3, 2025 08:41
@jamiecobbett
Copy link
Contributor Author

I've also noticed that if you use the root: option, then it seems the paths in glob: don't take that into account - ie you must specify the full path, whereas I assumed it would be relative to the root.

I've gone ahead and added that to this PR, but please shout if you'd prefer it to be broken off.

@mrexox mrexox merged commit d87b8d4 into evilmartians:master Feb 4, 2025
11 of 12 checks passed
@jamiecobbett jamiecobbett deleted the add-notes-about-double-asterisk-glob-behaviour branch February 4, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants