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

fix: use lstat bigint=true for local environments to fix #1245 #1778

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
11 changes: 11 additions & 0 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,17 @@
"doc",
"test"
]
},
{
"login": "jayree",
"name": "jayree",
"avatar_url": "https://avatars.githubusercontent.com/u/14836154?v=4",
"profile": "https://github.com/jayree",
"contributions": [
"code",
"doc",
"test"
]
}
],
"commitConvention": "angular"
Expand Down
3 changes: 3 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,8 @@
],
"newlines-between": "always"
}]
},
"env": {
"es2020": true
}
}
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ Thanks goes to these wonderful people ([emoji key](https://github.com/kentcdodds
<td align="center"><a href="https://daniel-ruf.de"><img src="https://avatars1.githubusercontent.com/u/827205?v=4&s=60?s=60" width="60px;" alt=""/><br /><sub><b>Daniel Ruf</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=DanielRuf" title="Code">💻</a></td>
<td align="center"><a href="http://blog.bokuweb.me/"><img src="https://avatars0.githubusercontent.com/u/10220449?v=4&s=60?s=60" width="60px;" alt=""/><br /><sub><b>bokuweb</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=bokuweb" title="Code">💻</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=bokuweb" title="Documentation">📖</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=bokuweb" title="Tests">⚠️</a></td>
<td align="center"><a href="https://github.com/hirokiosame"><img src="https://avatars0.githubusercontent.com/u/1075694?v=4&s=60?s=60" width="60px;" alt=""/><br /><sub><b>Hiroki Osame</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=hirokiosame" title="Code">💻</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=hirokiosame" title="Documentation">📖</a></td>
<td align="center"><a href="https://jcubic.pl/me"><img src="https://avatars1.githubusercontent.com/u/280241?v=4&s=60?s=60" width="60px;" alt=""/><br /><sub><b>Jakub Jankiewicz</b></sub></a><br /><a href="#question-jcubic" title="Answering Questions">💬</a> <a href="https://github.com/isomorphic-git/isomorphic-git/issues?q=author%3Ajcubic" title="Bug reports">🐛</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=jcubic" title="Code">💻</a> <a href="#example-jcubic" title="Examples">💡</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=jcubic" title="Tests">⚠️</a></td>
<td align="center"><a href="http://jcubic.pl/me"><img src="https://avatars1.githubusercontent.com/u/280241?v=4&s=60?s=60" width="60px;" alt=""/><br /><sub><b>Jakub Jankiewicz</b></sub></a><br /><a href="#question-jcubic" title="Answering Questions">💬</a> <a href="https://github.com/isomorphic-git/isomorphic-git/issues?q=author%3Ajcubic" title="Bug reports">🐛</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=jcubic" title="Code">💻</a> <a href="#example-jcubic" title="Examples">💡</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=jcubic" title="Tests">⚠️</a></td>
</tr>
<tr>
<td align="center"><a href="https://github.com/howardgod"><img src="https://avatars1.githubusercontent.com/u/10459637?v=4&s=60?s=60" width="60px;" alt=""/><br /><sub><b>howardgod</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/issues?q=author%3Ahowardgod" title="Bug reports">🐛</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=howardgod" title="Code">💻</a></td>
Expand Down Expand Up @@ -361,6 +361,7 @@ Thanks goes to these wonderful people ([emoji key](https://github.com/kentcdodds
<td align="center"><a href="https://gitlab.com/CoalZombik/"><img src="https://avatars.githubusercontent.com/u/49895741?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Lukáš Cezner</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/issues?q=author%3ACoalZombik" title="Bug reports">🐛</a></td>
<td align="center"><a href="https://github.com/dead-end"><img src="https://avatars.githubusercontent.com/u/30635084?v=4?s=60" width="60px;" alt=""/><br /><sub><b>dead-end</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=dead-end" title="Code">💻</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=dead-end" title="Documentation">📖</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=dead-end" title="Tests">⚠️</a></td>
<td align="center"><a href="https://github.com/barry963"><img src="https://avatars.githubusercontent.com/u/5289896?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Barry</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=barry963" title="Code">💻</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=barry963" title="Documentation">📖</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=barry963" title="Tests">⚠️</a></td>
<td align="center"><a href="https://github.com/jayree"><img src="https://avatars.githubusercontent.com/u/14836154?v=4?s=60" width="60px;" alt=""/><br /><sub><b>jayree</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=jayree" title="Code">💻</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=jayree" title="Documentation">📖</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=jayree" title="Tests">⚠️</a></td>
</tr>
</table>

Expand Down
11 changes: 11 additions & 0 deletions __tests__/test-GitIndex.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,15 @@ describe('GitIndex', () => {
expect(index.entriesMap.get('c').stages.length).toBe(1)
})
})

it('ensure nanoseconds are always present', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good idea to write a test that addresses this issue. We can create a repository with git that contains a file and add it to the test repos and then compare the isomorphic-git lstat values with the values in the index file.

@dead-end We can try to enhance this one but I don't know if we can hardcode timestamps to test them the way you want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you create a repo with git, then this should have an index file with the timestamps. Call GitIndex.fromBuffer to get the index content and then call lstat for each entry and compare the values. I think this should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dead-end tried it with this approach: e6c277a but as 'nano.txt' is checked out on the build server, the timestamp changes, so the test will always fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayree Sorry, I did not think of that. The only possibilities I can think of are calling git from node or put the nano.txt in a zipfile and unzip the file during the test run (this should preserve the timestamps). I do not know if this is possible.
@jcubic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dead-end I'm simply using the isomorphic-git repo now

const { fs, dir } = await makeFixture('test-GitIndex')
const buffer = await fs.read(path.join(dir, 'simple-index'))
const index = await GitIndex.from(buffer)

const ctimeSeconds = index.entriesMap.get('world.txt').ctimeSeconds
const ctimeNanoseconds = index.entriesMap.get('world.txt').ctimeNanoseconds
expect(ctimeSeconds).not.toBeNull()
expect(ctimeNanoseconds).not.toBeNull()
})
})
8 changes: 4 additions & 4 deletions __tests__/test-abortMerge.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ describe('abortMerge', () => {
fs,
dir,
gitdir,
ours: 'A',
theirs: 'B',
ours: 'c',
theirs: 'd',
abortOnConflict: false,
author: {
name: 'Mr. Test',
Expand All @@ -195,9 +195,9 @@ describe('abortMerge', () => {
await fs.write(`${dir}/b/b`, 'new text for file b')
await fs.write(`${dir}/c/c`, 'new text for file c')

await abortMerge({ fs, dir, gitdir, commit: 'A' })
await abortMerge({ fs, dir, gitdir, commit: 'c' })

const trees = [TREE({ ref: 'A' }), WORKDIR(), STAGE()]
const trees = [TREE({ ref: 'c' }), WORKDIR(), STAGE()]
await walk({
fs,
dir,
Expand Down
22 changes: 22 additions & 0 deletions __tests__/test-status.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,26 @@ describe('status', () => {
const b = await status({ fs, dir, gitdir, filepath: 'b.txt' })
expect(b).toEqual('added')
})

/**
* The unit test checks the fix for:
*
* https://github.com/isomorphic-git/isomorphic-git/issues/1245
*
* Without the fix, the second write is not detected and the status is 'added'
* instead of '*added'
*/
it('status of a file changed twice within a second', async () => {
const { fs, dir } = await makeFixture('test-empty')
const file = 'a.txt'

const start = Date.now()
await fs.write(path.join(dir, file), 'Hi')
await add({ fs, dir, filepath: file })
await fs.write(path.join(dir, file), 'Ho')

const a = await status({ fs, dir, filepath: file })
expect(a).toEqual('*added')
expect(Date.now() - start).toBeLessThan(1000)
})
})
5 changes: 4 additions & 1 deletion jest.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@ module.exports = {
'jest-junit',
{
outputDirectory: 'junit',
outputName: `TESTS-node-${process.version}-${process.platform}-${require('os').release()}.xml`,
outputName: `TESTS-node-${process.version}-${
process.platform
}-${require('os').release()}.xml`,
},
],
],
coverageReporters: ['lcov', 'cobertura'],
setupFilesAfterEnv: ['./jest.setup.js'],
}
2 changes: 2 additions & 0 deletions jest.setup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/* eslint-disable no-undef */
jest.retryTimes(3, { logErrorsBeforeRetry: true })
6 changes: 6 additions & 0 deletions src/models/FileSystem.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,12 @@ export class FileSystem {
async lstat(filename) {
try {
const stats = await this._lstat(filename)
// For non-browser (local) scenarios regular git 'add' command might be used to write the index. Therefore we need to have the exact nanoseconds (see. normalizeStats.js SecondsNanoseconds ).
Copy link
Contributor Author

@jayree jayree Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think the issue occurs if users use their repository with git and isomorphic-git. If you stage a file with git and then call statusMatrix with isomorphic-git then you compare the git lstat with the isomorphic-git lstat and there can be differences. And if you have differences then you throw away the cache and get a performance issue.

@dead-end thats right, and is mentioned here. if you use native git the timestamp (nanoseconds) is more accurate than the timestamp ('faked' nanoseconds by using ms * 1000000) in isomorphic-git. (this is also the reason why the git index is broken after isomorphic-git tries to rebuild it, becuase it uses the 'faked' nanoseconds)

Thats why we need to differentiate between browser and local. With bigint: true isomorphic-git is able to create the same timestamp (nanoseconds) as native git does but this option is not supported by browsers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayree You have written:

(this is also the reason why the git index is broken after isomorphic-git tries to rebuild it, becuase it uses the 'faked' nanoseconds)

Can you explain how the faked nanos lead to the broken index file? We have a lot of issues with this and I would like to understand it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dead-end with the old behaviour nanoseconds were calculated based on ms.

From a file with mtimeMs: 1683216016493.005
you'll get 1683216016s and 493.0048828125ms --> 493004882.8125ns
Due to the numeric data types, floating point values are used, which is not the case in the BigInt data structure.
If you use the mtimeNs: 1683216016493004997n (bigint) value
you'll get 1683216016s and 493004997ns. These are the vaules git uses in the index.

The old logic fixing the #1245 compared 493004882.8125ns with 493004997ns which caused Isomorphic-git to think the index is broken telling isomorphic-git to rebuild the index, but with 1683216016s 493004882.8125ns wich doesn't match the real ns timestamp of the file.

if (!process.browser) {
const statsNs = await this._lstat(filename, { bigint: true })
stats.mtimeNs = statsNs.mtimeNs
stats.ctimeNs = statsNs.ctimeNs
}
return stats
} catch (err) {
if (err.code === 'ENOENT') {
Expand Down
2 changes: 2 additions & 0 deletions src/utils/compareStats.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ export function compareStats(entry, stats) {
e.mode !== s.mode ||
e.mtimeSeconds !== s.mtimeSeconds ||
e.ctimeSeconds !== s.ctimeSeconds ||
e.mtimeNanoseconds !== s.mtimeNanoseconds ||
e.ctimeNanoseconds !== s.ctimeNanoseconds ||
e.uid !== s.uid ||
e.gid !== s.gid ||
e.ino !== s.ino ||
Expand Down
19 changes: 15 additions & 4 deletions src/utils/normalizeStats.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,41 @@ const MAX_UINT32 = 2 ** 32
function SecondsNanoseconds(
givenSeconds,
givenNanoseconds,
nanoseconds,
milliseconds,
date
) {
if (givenSeconds !== undefined && givenNanoseconds !== undefined) {
return [givenSeconds, givenNanoseconds]
}
if (milliseconds === undefined) {
milliseconds = date.valueOf()
let seconds
// For browser scenarios isomorphic-git 'add' will be used to write the index. Reading and writing are handled using normalizeStats ( see FileSystem.js lstat() ).
if (nanoseconds === undefined) {
milliseconds = milliseconds || date.valueOf()
seconds = Math.trunc(milliseconds / 1000)
nanoseconds = (milliseconds - seconds * 1000) * 1000000 // nanoseconds with millisecond precision
}
const seconds = Math.floor(milliseconds / 1000)
const nanoseconds = (milliseconds - seconds * 1000) * 1000000
// For non-browser (local) scenarios
else {
seconds = Number(nanoseconds / BigInt(1e9))
nanoseconds = Number(nanoseconds % BigInt(1e9))
}

return [seconds, nanoseconds]
}

export function normalizeStats(e) {
const [ctimeSeconds, ctimeNanoseconds] = SecondsNanoseconds(
e.ctimeSeconds,
e.ctimeNanoseconds,
e.ctimeNs,
e.ctimeMs,
e.ctime
)
const [mtimeSeconds, mtimeNanoseconds] = SecondsNanoseconds(
e.mtimeSeconds,
e.mtimeNanoseconds,
e.mtimeNs,
e.mtimeMs,
e.mtime
)
Expand Down