File fail on overwrite by KevDi · Pull Request #2842 · pocoproject/poco

KevDi · 2019-11-18T12:37:26Z

With the optional Parameter failOnOverwrite it's possible to get an exception if the destination File exists on copy/move/rename. In some cases it is usefull to get the Information if the Copy/Move/Rename fail because of an existing File.

obiltschnig · 2019-11-25T10:55:31Z

How about, instead of using a bool parameter, using an options parameter with a FAIL_ON_OVERWRITE flag? Would make the code much more readable. I know we have used bool parameters in way too many places in Poco already, but better late then never to start making it right.

Something like:

enum Options
{
    OPT_FAIL_ON_OVERWRITE = 0x01
};

...

void copyTo(const std::string& path, int options = 0) const;

KevDi · 2019-11-26T05:25:00Z

I could change it to an Option Parameter like you showed if you want.
But where should i place the enum? Inside the File.h? Or should it be placed in every File_* Header?

obiltschnig · 2019-11-26T09:22:52Z

Just put the Options enum inside the File class definition in File.h:

class Foundation_API File: private FileImpl
	/// The File class provides methods for working with a file.
	/// ...
{
public:
	typedef FileSizeImpl FileSize;

	enum LinkType
		/// Type of link for linkTo().
	{
		LINK_HARD     = 0, /// hard link
		LINK_SYMBOLIC = 1  /// symbolic link
	};

	enum Options
	{
		OPT_FAIL_ON_OVERWRITE = 0x01
	};

...

…i/poco into feature/FileFailOnOverwrite

KevDi · 2019-11-26T12:05:55Z

So i have added the Options Advice. Pls take a look and say if it's okay so =)

obiltschnig

Looks good, but please fix the comments still mentioning the failOnOverwrite param.

obiltschnig

I have some more comments/requests for change, unfortunately.

obiltschnig · 2019-11-26T13:36:12Z

Foundation/include/Poco/File_WINCE.h

 	void setExecutableImpl(bool flag = true);
-	void copyToImpl(const std::string& path) const;
-	void renameToImpl(const std::string& path);
+	void copyToImpl(const std::string& path, bool failOnOverwrite = false) const;


Needs to be fixed.

obiltschnig · 2019-11-26T13:36:58Z

Foundation/src/File_WINCE.cpp



-void FileImpl::copyToImpl(const std::string& path) const
+void FileImpl::copyToImpl(const std::string& path, bool failOnOverwrite) const


Needs to be fixed.

obiltschnig · 2019-11-26T13:37:14Z

Foundation/include/Poco/File_WINCE.h

-	void copyToImpl(const std::string& path) const;
-	void renameToImpl(const std::string& path);
+	void copyToImpl(const std::string& path, bool failOnOverwrite = false) const;
+	void renameToImpl(const std::string& path, bool failOnOverwrite = false);


obiltschnig · 2019-11-26T13:38:20Z

Foundation/src/File_UNIX.cpp

-
-	int dd = open(path.c_str(), O_CREAT | O_TRUNC | O_WRONLY, st.st_mode);
+	int dd;
+	if (options == 1) {


better check would be options & File::OPT_FAIL_ON_OVERWRITE (in case we add another option)

So i have to include the File.h Header inside the File_UNIX.cpp?
Which seems like a circle include because File_UNIX already included inside the File.h

obiltschnig · 2019-11-26T13:38:45Z

Foundation/src/File_VX.cpp


-	int dd = open(path.c_str(), O_CREAT | O_TRUNC | O_WRONLY, st.st_mode & S_IRWXU);
+	int dd;
+	if (options == 1) {


options & File::OPT_FAIL_ON_OVERWRITE

Foundation/src/File_VX.cpp

obiltschnig · 2019-11-26T13:39:35Z

Foundation/src/File_WIN32.cpp

 	poco_assert (!_path.empty());

-	if (CopyFileA(_path.c_str(), path.c_str(), FALSE) == 0)
+	if (CopyFileA(_path.c_str(), path.c_str(), options == 1) == 0)


(options & File::OPT_FAIL_ON_OVERWRITE) != 0

obiltschnig · 2019-11-26T13:39:53Z

Foundation/src/File_WIN32.cpp


-	if (MoveFileExA(_path.c_str(), path.c_str(), MOVEFILE_REPLACE_EXISTING) == 0)
-		handleLastErrorImpl(_path);
+	if (options == 1) {


options & File::OPT_FAIL_ON_OVERWRITE

obiltschnig · 2019-11-26T13:40:08Z

Foundation/src/File_WIN32U.cpp

 	std::wstring upath;
 	convertPath(path, upath);
-	if (CopyFileW(_upath.c_str(), upath.c_str(), FALSE) == 0)
+	if (CopyFileW(_upath.c_str(), upath.c_str(), options == 1) == 0)


(options & File::OPT_FAIL_ON_OVERWRITE) != 0

obiltschnig · 2019-11-26T13:40:14Z

Foundation/src/File_WIN32U.cpp

 	convertPath(path, upath);
-	if (MoveFileExW(_upath.c_str(), upath.c_str(), MOVEFILE_REPLACE_EXISTING) == 0)
-		handleLastErrorImpl(_path);
+	if (options == 1) {


options & File::OPT_FAIL_ON_OVERWRITE

KevDi · 2019-11-26T14:59:34Z

So i have to include the File.h Header inside the File_UNIX.cpp?
Which seems like a circle include because File_UNIX already included inside the File.h

obiltschnig · 2019-11-26T15:22:37Z

Yeah, good point. In the other places we have actually defined the enum in the Impl classes (like FileSize). I see that for LinkType, this has also been implemented the "wrong" way, using a magic number in FileImpl, rather than the symbolic value. I'd say, leave it as is for now, I will eventually take care of both. And thank you for your contribution!

obiltschnig · 2019-11-26T15:23:23Z

Btw, an example for how it's done properly is the Thread class.

KevDi · 2019-11-27T13:46:47Z

I have added the remarks like it was implemented inside the Thread Class. Currently i done it for the Unix and Win32U File Implementation. Please take a look if it is ok so. If its ok i will made the changes for the missing ones.

obiltschnig · 2019-11-27T19:55:09Z

Looks good, but please also fix the checks for options == 1.

KevDi · 2019-11-27T20:02:21Z

Yes i will fix them i just want to ask if its okay the way i did it now

…nto feature/FileFailOnOverwrite

KevDi · 2019-11-30T12:57:16Z

I have made the changes to the other File Impls

Jan Kevin Dick and others added 6 commits November 18, 2019 13:06

Added FailOnOverwrite to File Class

4e73455

Added Tests

062a662

Refactor

1eab6cf

Fixed wrong method parameter

8b4ef5f

Fixed File on Unix to pass the Rename Tests

6c662bf

Fixed missing failOnOverwrite check

936441d

obiltschnig added this to the Release 1.10.0 milestone Nov 25, 2019

obiltschnig added the enhancement label Nov 25, 2019

obiltschnig self-assigned this Nov 25, 2019

Jan Kevin Dick added 2 commits November 26, 2019 13:03

Added the Options Enum advice.

d5a25f2

Merge branch 'feature/FileFailOnOverwrite' of https://github.com/KevD…

3424ee7

…i/poco into feature/FileFailOnOverwrite

obiltschnig requested changes Nov 26, 2019

View reviewed changes

Updated Comments and fixed bug within the Unix File Implementation

9daaec4

obiltschnig requested changes Nov 26, 2019

View reviewed changes

Added Fix to put Options inside the File Impl Classes

0a95250

KevDi added 3 commits November 30, 2019 13:21

Changes

7f3271f

Merge branch 'feature/FileFailOnOverwrite' of github.com:KevDi/poco i…

f6f8ef8

…nto feature/FileFailOnOverwrite

Changed Options Check in remaining File Impl Classes

fa20e05

obiltschnig merged commit 56fe4ea into pocoproject:poco-1.10.0 Dec 9, 2019



		void FileImpl::copyToImpl(const std::string& path) const
		void FileImpl::copyToImpl(const std::string& path, bool failOnOverwrite) const

Conversation

KevDi commented Nov 18, 2019

Uh oh!

obiltschnig commented Nov 25, 2019

Uh oh!

KevDi commented Nov 26, 2019

Uh oh!

obiltschnig commented Nov 26, 2019

Uh oh!

KevDi commented Nov 26, 2019

Uh oh!

obiltschnig left a comment

Choose a reason for hiding this comment

Uh oh!

obiltschnig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevDi Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevDi commented Nov 26, 2019

Uh oh!

obiltschnig commented Nov 26, 2019

Uh oh!

obiltschnig commented Nov 26, 2019

Uh oh!

KevDi commented Nov 27, 2019

Uh oh!

obiltschnig commented Nov 27, 2019

Uh oh!

KevDi commented Nov 27, 2019

Uh oh!

KevDi commented Nov 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KevDi Nov 26, 2019 •

edited

Loading