tinyglobby icon indicating copy to clipboard operation
tinyglobby copied to clipboard

gitignore support

Open benmccann opened this issue 1 year ago • 10 comments

from https://github.com/nitrojs/nitro/pull/3108#issuecomment-2669919095:

it has a big difference from globby where it does not have integration with npm ignore to auto support gitignore and other repo ignore paths (gitignore patterns are not same as glob ignore patterns, and they are relative to git root)

benmccann avatar Feb 19 '25 22:02 benmccann

i feel like #32 could probably work for these use cases

SuperchupuDev avatar Feb 19 '25 23:02 SuperchupuDev

Thanks for the issue and quick replies.

nagated support would be cool however gitignore and glob ignore patterns are different (gitignore patterns are absolute paths relative to git root like /test/* which need to be translated to <glob_base>/**, I assume there are be more diff like this)

For the context of my experience, With https://github.com/unjs/mkdist/pull/265 I tried to workaround, however, regressions like https://github.com/unjs/unbuild/issues/472 happened which finally had to do completely ignore gitignore story (https://github.com/unjs/mkdist/pull/279)

If supporting gitignore feels out of this project scope, I can understand, however it feel like a really missing part to make tinyglobby full replacement of globby

pi0 avatar Feb 19 '25 23:02 pi0

supporting a globby option is not out of the scope of this project, but i'm not sure how feasible it would be to implement it without adding any more dependencies (minimal dependencies is the other goal of this project). so i'm open to adding it as long as no new deps are introduced (even if the gitignore feature is used by many, it's still a really low % of usages compared to total globby usages (i have not verified this but i remember it being this way))

SuperchupuDev avatar Feb 20 '25 00:02 SuperchupuDev

We were already talking about it in #70 . globby uses ignore for its gitignore option while also handling everything itself.

But it's essentially: check for option -> find gitignore file -> parse file content -> use ignore for each line inside the gitignore file.

Torathion avatar Feb 20 '25 12:02 Torathion

From reading the docs, I wonder the ignore package handles this as expected: https://github.com/kaelzhang/node-ignore/tree/master?tab=readme-ov-file#2-filenames-and-dirnames

That's just not how gitignore patterns work. From https://git-scm.com/docs/gitignore:

If there is a separator at the end of the pattern then the pattern will only match directories, otherwise the pattern can match both files and directories.

I might be wrong, but from this I gather it wouldn't make sense to use the ignore package while claiming to support .gitignore files properly.

negated support would be cool however gitignore and glob ignore patterns are different (gitignore patterns are absolute paths relative to git root like /test/* which need to be translated to <glob_base>/**, I assume there are be more diff like this)

Agreed with this, insofar I think it should be translated to both <glob_base> and <glob_base>/** (ugly but we don't want to do fs ops).

For the record, Knip uses an implementation and some tests around this topic:

Nothing to be proud of, but does seem to do the job.

Just a shame there's no glob lib that supports negated ignore patterns 😬😬

supporting a globby option is not out of the scope of this project, but i'm not sure how feasible it would be to implement it without adding any more dependencies (minimal dependencies is the other goal of this project). so i'm open to adding it as long as no new deps are introduced (even if the gitignore feature is used by many, it's still a really low % of usages compared to total globby usages (i have not verified this but i remember it being this way))

Such numbers are hard to get by, but usage of gitignore: true is not zero (see e.g. ¹). But as long as tinyglobby is not a drop-in replacement (which is absolutely fine, I think we all can get behind tinyglobby's vision) it might be worth considering clarifying this in tinyglobby's docs. Because users often won't notice. Not sure how to call it, something with "ignorance" and "confirmation bias" on the user's end, idk.

¹ https://github.com/search?q=globby+%22gitignore%3A+true%22+%28language%3ATypeScript+OR+language%3AJavaScript%29&type=code

webpro avatar Mar 06 '25 08:03 webpro

Maybe it's interesting to consider this approach: instead of adding the gitignore: boolean option, model it after Node.js' built-in fs.glob options.exclude function. And thus keep the complexity out of tinyglobby.

Then, we could do something like this (simplified):

import tinyglobby from 'tinyglobby';
import ignore from 'ignore';

const files = tinyglobby(["**/*.js"], {
  exclude: filePath => ignore(filePath)
});

There are still some concerns with this approach, I can think of at least:

  1. The API, valid options include:
    1. add exclude as a function (like fs.glob), or
    2. extend ignore to also accept a function
  2. Still requires a match lib that supports to "unignore" → i.e. like #70 with unmatch yet not with the internally created unignoreMatcher function, but using the user-provided function of the previous point 1. instead

The main idea is to basically pass on the exclude (or ignore) function to the underlying matcher lib.

This has the benefit that users can use node-ignore, something else, experiment with it, etc. It will also ease support for cq the migration to the Node.js built-in fs.glob.

WDYT?

I could try to whip up some PR + examples when I'll find some time. But only if there's interest and we have agreement on point 2.

webpro avatar Mar 06 '25 12:03 webpro

James pointed out that this functionality might need to live in fdir. Also, to avoid bloating the library he suggested an exclude function and having the .gitignore functionality live in a separate library. I think this is reasonable since globby does not enable the functionality by default

benmccann avatar May 05 '25 14:05 benmccann

It's probably something like this snippet from @outslept:

import { glob } from 'tinyglobby';
import { execSync } from 'child_process';

async function globWithGitignore(patterns, options = {}) {
  const { cwd = process.cwd(), ...restOptions } = options;
  
  try {
    const gitIgnored = execSync(
      'git ls-files --others --ignored --exclude-standard --directory',
      { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }
    )
    .split('\n')
    .filter(Boolean)
    .map(file => file.replace(/\/$/, ''));

    return await glob(patterns, { 
      ...restOptions, 
      cwd, 
      ignore: [...(restOptions.ignore || []), ...gitIgnored]
    });
  } catch {
    return await glob(patterns, options);
  }
}

Additional comments from Tomer:

I think what @outslept sent looks basically right although maybe it needs to call convertPathToPattern on each item in gitIgnored?

I might also leave out the try-catch. Seems a little odd to silently ignore (ha!) the gitignores

Something this implementation does not do, however, is handle the case of globbing across multiple git projects, but I also feel like no one using this gitignore: true option of globby needs that (I certainly don't need it)

This discussion can be found on the e18e Discord

benmccann avatar Sep 02 '25 19:09 benmccann

(this will be documented on the upcoming website as well 👀)

outslept avatar Sep 02 '25 19:09 outslept

it has in fact just been documented on the website that has just been released https://superchupu.dev/tinyglobby/migration#gitignore

SuperchupuDev avatar Sep 06 '25 22:09 SuperchupuDev