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

curl: add command line option to prohibit overwriting files #4439

Closed
wants to merge 8 commits into from
Closed

curl: add command line option to prohibit overwriting files #4439

wants to merge 8 commits into from

Conversation

cvengler
Copy link
Contributor

About

This adds the parameter '--override' and '--no-override'.
If '--no-override' is set, file eutputs will fail if the file
already exists.
'--override' will change nothing.

Testing (Unix)

This happens in the root git directory.
Please adjust it to your system/config.

touch test
src/curl --no-override -o test https://www.emilengler.com

This will result in

curl: option -o: file exists and override is blocked
curl: try 'curl --help' or 'curl --manual' for more information

P.S: This is my first PR to this project so please correct me if I did some mistakes :)

This adds the parameter '--override' and '--no-override'.
If '--no-override' is set, file eutputs will fail if the file
already exists.
'--override' will change nothing.
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Thanks!

To complete the work and have it "mergable", you also need to add documentation (see docs/cmdline-opts) and at least one test case that verifies it.

src/tool_getparam.c Outdated Show resolved Hide resolved
src/tool_getparam.c Outdated Show resolved Hide resolved
src/tool_getparam.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Sep 29, 2019

The CI failure is test 1139 which fails because the lack of docs for the flag.

@dfandrich
Copy link
Contributor

dfandrich commented Sep 29, 2019 via email

@cvengler
Copy link
Contributor Author

@bagder What do you mean by lack of docs?
I have a docs/cmdline-opts/override.d

@bagder
Copy link
Member

bagder commented Sep 29, 2019

I have a docs/cmdline-opts/override.d

Oops, my mistake!

@cvengler
Copy link
Contributor Author

@dfandrich 'override' is much more common.

grep -ro 'override' | wc -l
726
grep -ro 'overwrite' | wc -l
53

@cvengler
Copy link
Contributor Author

@bagder Looks like it was malformed because I forgot an empty line at the end. Fixed it with 4850e30

@dfandrich
Copy link
Contributor

dfandrich commented Sep 30, 2019 via email

@jzakrzewski
Copy link
Contributor

I'm with @dfandrich.
Basically one overrides an option but overwrites file/data.

@@ -42,6 +42,9 @@
#include "tool_parsecfg.h"
#include "tool_main.h"

/* to use access() */
#include <unistd.h>

Copy link
Member

Choose a reason for hiding this comment

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

#ifdef HAVE_UNISTD_H
#include <unistd.h>
#endif

@bagder
Copy link
Member

bagder commented Sep 30, 2019

The scan-build also warns:

tool_getparam.c:505:8: warning: Null pointer passed as an argument to a 'nonnull' parameter
    if(access(fname, F_OK) != -1)
       ^~~~~~~~~~~~~~~~~~~

Help: Permit/Prohibit overwriting of files
---
If you route the output to a file which already exists, it can be disabled using
--no-override. The --override argument is the default.
Copy link
Member

Choose a reason for hiding this comment

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

Can I suggest a clearer wording that focuses on the use and the effects? Maybe something like this:

"This option allows curl to overwrite already existing files with the output options. If --no-overwrite is used, curl will instead return an error (which?) rather than overwriting an existing file. The default is to allow overwrites."

@cvengler
Copy link
Contributor Author

cvengler commented Sep 30, 2019

@jzakrzewski Ok, I will change this to overwrite.
@bagder, could you rename the PR title to
"[param]: Add param to prohibit overwriting files"

@bagder
Copy link
Member

bagder commented Sep 30, 2019

@emilengler you can change the title yourself, right?

@bagder
Copy link
Member

bagder commented Oct 1, 2019

Windows build failures:

3>.\tool_getparam.c(520) : error C2220: warning treated as error - no 'object' file generated
3>.\tool_getparam.c(520) : warning C4100: 'overwrite_file_prohibit' : unreferenced formal parameter
3>.\tool_getparam.c(520) : warning C4100: 'fname' : unreferenced formal parameter

@bagder bagder changed the title [param]: Add param to prohibit overriding files curl: add command line option to prohibit overwriting files Oct 1, 2019
@cvengler
Copy link
Contributor Author

cvengler commented Oct 1, 2019

@bagder No, only people with write access can change the title of a PR.
I will take a look at the windows errors soon

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

Generally I'm not a fan of the way this was implemented, and does it serve a purpose to do prohibit overwriting a header filename or trace filename etc?

Comment on lines 519 to 527
#ifndef HAVE_UNISTD_H
static bool CheckFile(char *fname, bool overwrite_file_prohibit)
{
/* Make this function useless if unistd is not available */
printf("overwrite parameter is being ignored because unistd.h is missing\n");
return TRUE;
}
#endif

Copy link
Member

Choose a reason for hiding this comment

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

we can't do printfs from these functions, it would need a warnf global I think. but a better idea is use stat or something else when access is not available

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stat still needs unistd.h I think

src/tool_help.c Outdated Show resolved Hide resolved
@cvengler
Copy link
Contributor Author

Push

@jay
Copy link
Member

jay commented Oct 31, 2019

I still don't like this idea, I think it is unnecessary to make it part of curl. It is easy enough in script to see if a file exists. In batch there's if not exist "filename" and in shell there's if [ ! -e "filename" ]

@cvengler
Copy link
Contributor Author

Will close this because of inactivity.
Leaving this with up for grabs if someone is interested in taking it over

@cvengler cvengler closed this Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants