Skip to content

Commit

Permalink
Extract .gitignore from hosted tarball for prepare
Browse files Browse the repository at this point in the history
Occasionally, a package installed from a hosted git repo requires its
.gitignore in order to properly run its install/prepare process.  Since
we should treat git dependencies as being just like installing from an
install cut from the actual git repo itself, it's inappropriate to munge
the .gitignore into .npmignore at this step (even though it _is_
appropriate to do so in most other cases with remote tarballs).

This adds the `allowGitIgnore` flag, which is then in turn used by the
GitFetcher when it makes its call to RemoteFetcher to download and
extract the hosted tarball for preparation.

This was not an issue in npm v6, because prior to npm v7, hosted
git snapshot tarballs were not used, so it never would go through the
code path of using a 'remote' type fetcher to download the contents of a
git repository (which was also much slower).

Fixes: npm/cli#2144
  • Loading branch information
isaacs committed Jan 19, 2021
1 parent 1ed13c0 commit 11c0526
Show file tree
Hide file tree
Showing 8 changed files with 41 additions and 2 deletions.
4 changes: 3 additions & 1 deletion lib/fetcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ class FetcherBase {
throw new TypeError('options object is required')
this.spec = npa(spec, opts.where)

this.allowGitIgnore = !!opts.allowGitIgnore

// a bit redundant because presumably the caller already knows this,
// but it makes it easier to not have to keep track of the requested
// spec when we're dispatching thousands of these at once, and normalizing
Expand Down Expand Up @@ -414,7 +416,7 @@ class FetcherBase {
const base = basename(entry.path)
if (base === '.npmignore')
sawIgnores.add(entry.path)
else if (base === '.gitignore') {
else if (base === '.gitignore' && !this.allowGitIgnore) {
// rename, but only if there's not already a .npmignore
const ni = entry.path.replace(/\.gitignore$/, '.npmignore')
if (sawIgnores.has(ni))
Expand Down
1 change: 1 addition & 0 deletions lib/git.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ class GitFetcher extends Fetcher {
const nameat = this.spec.name ? `${this.spec.name}@` : ''
return new RemoteFetcher(h.tarball({ noCommittish: false }), {
...this.opts,
allowGitIgnore: true,
pkgid: `git:${nameat}${this.resolved}`,
resolved: this.resolved,
integrity: null, // it'll always be different, if we have one
Expand Down
Binary file not shown.
1 change: 1 addition & 0 deletions test/fixtures/prepare-requires-gitignore/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# just a file you can ignore
1 change: 1 addition & 0 deletions test/fixtures/prepare-requires-gitignore/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('this is fine')
10 changes: 10 additions & 0 deletions test/fixtures/prepare-requires-gitignore/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "prepare-requires-gitignore",
"version": "1.2.3",
"files": [
"index.js"
],
"scripts": {
"prepare": "node prepare.js"
}
}
2 changes: 2 additions & 0 deletions test/fixtures/prepare-requires-gitignore/prepare.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
require('fs').statSync(`${__dirname}/.gitignore`)
console.log('this is fine')
24 changes: 23 additions & 1 deletion test/git.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ const http = require('http')

const {resolve} = require('path')
const rimraf = require('rimraf')
const abbrev = resolve(__dirname, 'fixtures/abbrev-1.1.1.tgz')
const fixtures = resolve(__dirname, 'fixtures')
const abbrev = resolve(fixtures, 'abbrev-1.1.1.tgz')
const prepIgnore = resolve(fixtures, 'prepare-requires-gitignore-1.2.3.tgz')
const npa = require('npm-package-arg')

t.saveFixture = true
Expand Down Expand Up @@ -250,6 +252,13 @@ t.test('setup', { bail: true }, t => {
case '/not-tar.tgz':
res.statusCode = 200
return res.end('this is not a gzipped tarball tho')
case '/prepare-requires-gitignore-1.2.3.tgz': try {
const data = fs.readFileSync(prepIgnore)
return res.end(data)
} catch (er) {
res.statusCode = 500
return res.end(er.stack)
}
default:
res.statusCode = 404
return res.end('not found')
Expand Down Expand Up @@ -389,6 +398,19 @@ t.test('extract from tarball from hosted git service', t => {
}))
})

t.test('include .gitignore in hosted tarballs for preparation', async t => {
const spec = npa(`localhost:foo/y#${REPO_HEAD}`)
spec.hosted.tarball = () =>
`http://localhost:${httpPort}/prepare-requires-gitignore-1.2.3.tgz`
const g = new GitFetcher(spec, {cache})
const dir = t.testdir()
await g.extract(dir)
t.strictSame(fs.readdirSync(dir).sort((a,b) => a.localeCompare(b)), [
'index.js',
'package.json',
])
})

t.test('add git sha to hosted git shorthand', t =>
new GitFetcher('localhost:repo/x', {cache})
// it adds the git+ because it thinks it's https
Expand Down

0 comments on commit 11c0526

Please sign in to comment.