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 #631 throw error when overwriting recently created file #702

Merged
merged 5 commits into from
Apr 15, 2017

Conversation

uttpal
Copy link
Contributor

@uttpal uttpal commented Apr 11, 2017

Fixes #631
If you have a copy command like:

$ cp file1.txt out/file1.txt destination/
You should get an error that says something like:

cp: will not overwrite just-created ‘destination/file1.txt’ with ‘out/file1.txt’

same behavior goes for mv with -R, -f flags.

@codecov-io
Copy link

codecov-io commented Apr 11, 2017

Codecov Report

Merging #702 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
- Coverage   94.67%   94.66%   -0.01%     
==========================================
  Files          33       33              
  Lines        1183     1219      +36     
==========================================
+ Hits         1120     1154      +34     
- Misses         63       65       +2
Impacted Files Coverage Δ
src/cp.js 91.05% <100%> (+0.7%) ⬆️
src/mv.js 97.77% <100%> (+0.48%) ⬆️
src/rm.js 97.01% <0%> (-2.99%) ⬇️
src/common.js 98.88% <0%> (-0.02%) ⬇️
src/which.js 94.87% <0%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80d590e...0495354. Read the comment docs.

Copy link
Member

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

@uttpal thanks for the PR! This looks good overall, but I think we're missing the case where we're copying a directory (with colliding contents).

I gave suggestions in the review comments. Let me know if I can clarify, or if I was mistaken about anything.



test('should not overwrite recently created files (in recursive Mode)', t => {
const result = shell.cp('-R', 'resources/file1', 'resources/cp/file1', t.context.tmp);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test for copying a non-empty directory. How about this?

cp('-R', 'resources/cp/a', 'resources/cp/', t.context.tmp)

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I don't think we need this test

@@ -150,6 +150,14 @@ function cpdirSyncRecursive(sourceDir, destDir, currentDepth, opts) {
} // for files
} // cpdirSyncRecursive

// Checks if cureent file was created recently
function checkRecentCreated(sources, index) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's be better to keep a list of all copied files, as they're copied (instead of relying on the parameter list). The parameter list might contain directory names (but we actually want a list of all files inside those directories)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nfischer Thanks for suggestions.
How about we use the timestamp to check whether a file was created recently or not?
we can set a variable with current time when cp operation starts and then only overwrite files that were created before that.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds trickier--if someone is playing with timestamps on their files, we might get different behavior. The goal is to just catch files created by the current cp command. I just suggested building a list of files we create, as we create them. Fairly confident this is how GNU cp does this.

Copy link
Member

Choose a reason for hiding this comment

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

It appears like relying on the parameter list is consistent with GNU cp, I must have been mistaken when I tried it

test/mv.js Outdated

result = shell.mv('t/file1', 'file1'); // revert
t.truthy(fs.existsSync('file1'));
t.truthy(fs.existsSync('cp/file1'));
Copy link
Member

Choose a reason for hiding this comment

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

Is this all to manually revert? I think our beforeEach step should be sufficient

Copy link
Member

Choose a reason for hiding this comment

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

LGTM % this comment

@nfischer
Copy link
Member

Thanks!

@nfischer nfischer merged commit bbe521b into shelljs:master Apr 15, 2017
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.

3 participants