fix fs.truncate when first arg is an int fd #9161
Conversation
|
Can you add a regression test please. |
|
@cjihrig Getting late I'll do it tomorrow. Where should I put it? I guess it is |
|
@cjihrig I added the test. |
| var path = require('path'); | ||
| var fs = require('fs'); | ||
| var tmp = common.tmpDir; | ||
| if (!fs.existsSync(tmp)) fs.mkdirSync(tmp); |
There was a problem hiding this comment.
Can you put the fs.mkdirSync(tmp); on a new line.
|
I notice that this behavior doesn't seem to be documented. Perhaps it should be added there. |
|
Regarding the doc issue, I have the impression that this is compatibility code. People should use |
| fs.writeFileSync(filename, 'hello world', 'utf8'); | ||
| var fd = fs.openSync(filename, 'r+'); | ||
| fs.truncate(fd, 5, function(err) { | ||
| if (err) throw err; |
There was a problem hiding this comment.
This can just be assert.equal(err, null); (or undefined depending what the success case is).
|
@tjfontaine should this behavior be documented? |
|
Yes, I think we should document this behavior, for v0.10 and v0.12. We can decide if we want to deprecate/remove this fallback in master at a later date. I agree that this fix is the the best way to handle the bug for v0.12. For master though, I think we want to fix
This would have made there result much more crisp and clear about what went wrong. |
|
@tjfontaine I agree. I can handle the doc issue if you want but |
|
@bjouhier can you address the last code nit, update the docs, and squash your commits. |
bjouhier
force-pushed
the
truncate-fix
branch
from
3a37851
to
75643fa
Compare
added unit test added note to the doc improved makeCallback arg checking
bjouhier
force-pushed
the
truncate-fix
branch
from
75643fa
to
3bcfc12
Compare
|
@cjihrig I simplified the unit test, improved |
Currently, fs.truncate() silently fails when a file descriptor is passed as the first argument. This commit changes this behavior to properly call fs.ftruncate(). This commit also adds proper type checking to the callback provided to makeCallback(). PR-URL: #9161 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy J Fontaine <[email protected]>
|
Thanks! Landed in master in 4c31cda with a few tweaks. |
Currently, fs.truncate() silently fails when a file descriptor is passed as the first argument. This commit changes this behavior to properly call fs.ftruncate(). PR-URL: #9161 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy J Fontaine <[email protected]>
|
And landed in 0.12 in b3aa876 without the change to |
Currently, fs.truncate() silently fails when a file descriptor is passed as the first argument. This commit changes this behavior to properly call fs.ftruncate(). PR-URL: nodejs/node-v0.x-archive#9161 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy J Fontaine <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Conflicts: lib/fs.js
Currently, fs.truncate() silently fails when a file descriptor is passed as the first argument. This commit changes this behavior to properly call fs.ftruncate(). This commit also adds proper type checking to the callback provided to makeCallback(). PR-URL: nodejs#9161 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy J Fontaine <[email protected]>
Bug is rather obvious and fix is trivial (see diff below).