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

[filesystem] handle copying when source and target destinations are the same #4813

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

vince-fugnitto
Copy link
Member

Fixes #4812

  • Fixes an issue when calling copy from the filesystem when
    the source and target destinations are the same. Currently,
    no check is performed to verify if the paths are different
    which leads to fs-extra throwing an error.

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added bug bugs found in the application filesystem issues related to the filesystem labels Apr 4, 2019
@akosyakov akosyakov requested a review from kittaakos April 5, 2019 06:13
@akosyakov
Copy link
Member

@kittaakos Could you review it please?

@kittaakos
Copy link
Contributor

@vince-fugnitto, test this, please.

I think it would be better to throw an error, instead of silently return with the source.

$ touch foo.txt
$ cp foo.txt foo.txt 
cp: foo.txt and foo.txt are identical (not copied).

Thoughts?

@vince-fugnitto
Copy link
Member Author

I think it would be better to throw an error, instead of silently return with the source.

Sure, I can throw an error instead!

@akosyakov
Copy link
Member

I think it would be better to throw an error, instead of silently return with the source.

We would need to check that all clients expect it and handle properly. Do you expect that client should do something special with such error or they would just ignore? If ignore maybe return silently is not a bad option, less code for clients or errors in logs.

@vince-fugnitto
Copy link
Member Author

I think it would be better to throw an error, instead of silently return with the source.

We would need to check that all clients expect it and handle properly. Do you expect that client should do something special with such error or they would just ignore? If ignore maybe return silently is not a bad option, less code for clients or errors in logs.

I was thinking silent would be enough since the error would most likely be ignored.
Copying a file to itself won't do anything anyways, so there's really no need to perform extra handling on each client. I returned the sourceStat since we can assume the copy was successful to
itself (does nothing)

@kittaakos
Copy link
Contributor

e enough since the error would most likely be ignored.

That's my problem, if you are copying something to the same destination, then something was already wrong. So I want to see an error, instead of ignoring it.

@vince-fugnitto
Copy link
Member Author

@kittaakos updated!

@kittaakos
Copy link
Contributor

@kittaakos updated!

Please add a test.

Fixes #4812

- Fixes an issue when calling `copy` from the filesystem when
the `source` and `target` destinations are the same. Currently,
no check is performed to verify if the paths are different
which leads to `fs-extra` throwing an error.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto
Copy link
Member Author

@kittaakos updated!

Please add a test.

Done!

@vince-fugnitto
Copy link
Member Author

@kittaakos are you fine with the latest changes?

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

It works as expected; thank you for the patch! 👍

@vince-fugnitto vince-fugnitto merged commit 27750e3 into master Apr 8, 2019
@vince-fugnitto vince-fugnitto deleted the vf/filesystem-copy branch April 8, 2019 14:23
@vince-fugnitto
Copy link
Member Author

thank you @kittaakos !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants