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

src: add CURLOPT_UPLOAD_FLAGS #15970

Closed
wants to merge 13 commits into from
Closed

src: add CURLOPT_UPLOAD_FLAGS #15970

wants to merge 13 commits into from

Conversation

tiymat
Copy link
Contributor

@tiymat tiymat commented Jan 11, 2025

Adds support for specifying upload flags, to allow e.g. uploading mail via IMAP without the Seen flag. e.g. curl imap://example.com:993/MAILBOX -u 'user@example.com' --upload-file /some/file --upload-flags !Seen,Flagged

I wasn't sure what to do about the fact that the existing IMAP append implementation sends the Seen flag by default. If I naively kept that behaviour and provided only the Answered, Deleted, Draft, Flagged, and Seen flags, there would be no way to turn the Seen flag off. There were a couple of approaches I considered.

  • Change the default behaviour not to send Seen unless specified with the new opt, but this would break existing scripts/commands.

  • Instead of allowing a Seen flag, make it an Unseen flag. This would let the user make an upload unseen but would be a little wonky as it works the opposite way of the other flags (disabling functionality rather than enabling it) and might be a problem if in the future, we want to be able to set an upload as Seen using a protocol other than IMAP.

  • Allow inversion of flags via a ! prefix. This was what I eventually chose. It's likely overkill for just the current set of flags (the only situation in which you'd need to use ! is !Seen, since the other flags are already unset by default) but works well if in the future there is a need to use the same flag names in other protocols.

If you have any thoughts on this please let me know.

@testclutch
Copy link

Analysis of PR #15970 at 55d889c4:

Test 1140 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 13 different CI jobs (the link just goes to one of them).

Test 1173 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 13 different CI jobs (the link just goes to one of them).

Test 3209 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 13 different CI jobs (the link just goes to one of them).

Test 3208 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 13 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@tiymat
Copy link
Contributor Author

tiymat commented Jan 11, 2025

The build failures surprise me as they aren't happening locally for me. They seem caused by my use of the strparse.h string manipulation functions in tool_getparam.c. I guess I can rewrite it to avoid calling those functions if needed.

ld: src/CMakeFiles/curl.dir/Unity/unity_0_c.c.o: in function `parse_upload_flags':
/home/runner/work/curl/curl/src/tool_getparam.c:1573: undefined reference to `Curl_str_single'
ld: /home/runner/work/curl/curl/src/tool_getparam.c:1614: undefined reference to `Curl_str_single'
ld: /home/runner/work/curl/curl/src/tool_getparam.c:1572: undefined reference to `Curl_str_until'
ninja: build stopped: subcommand failed.

The test cases I added (3208 and 3209) seem to be failing because of CRLF line endings

test 3208...[Upload message unread via IMAP]

 3208: protocol FAILED:
--- log/10/check-expected	2025-01-11 21:04:31.054289991 +0000
+++ log/10/check-generated	2025-01-11 21:04:31.054289991 +0000
@@ -1,4 +1,4 @@
-A001 CAPABILITY[LF]
-A002 LOGIN user secret[LF]
-A003 APPEND 3208 {356}[LF]
-A004 LOGOUT[LF]
+A001 CAPABILITY[CR][LF]
+A002 LOGIN user secret[CR][LF]
+A003 APPEND 3208 {356}[CR][LF]
+A004 LOGOUT[CR][LF]

I thought this was set in .gitattributes?

@bagder bagder added the feature-window A merge of this requires an open feature window label Jan 12, 2025
@bagder
Copy link
Member

bagder commented Jan 12, 2025

The test cases I added (3208 and 3209) seem to be failing because of CRLF line endings

You probably want your <protocol> tags to use <protocol crlf="yes">.

@tiymat
Copy link
Contributor Author

tiymat commented Jan 15, 2025

Thanks for the review! I've committed fixes for each of your remarks. Unfortunately it looks like there was a warning in the make output I didn't see, I think related to the data type change to unsigned char. Hopefully I'll be able to look at that sometime next week.

@tiymat
Copy link
Contributor Author

tiymat commented Jan 18, 2025

1140 and 1173 seem to be failing due to the absence of docs/libcurl/opts/CURLOPT_UPLOAD_FLAGS.3.

I included a docs/libcurl/opts/CURLOPT_UPLOAD_FLAGS.md file in my initial commit and have a CURLOPT_UPLOAD_FLAGS.3 locally, but that file is gitignored, so I don't know how to fix the tests.

[curl] > git check-ignore docs/libcurl/opts/CURLOPT_UPLOAD_FLAGS.3
docs/libcurl/opts/CURLOPT_UPLOAD_FLAGS.3
test 1140...[Verify the nroff of manpages]

 1140: stdout FAILED:
--- log/16/check-expected	2025-01-17 20:59:18.281534572 +0000
+++ log/16/check-generated	2025-01-17 20:59:18.281534572 +0000
@@ -1 +1,2 @@
-OK[LF]
+error: /home/runner/work/curl/curl/tests/../docs/libcurl/curl_easy_setopt.3:667: referring to non-existing manpage CURLOPT_UPLOAD_FLAGS.3[LF]
+error: /home/runner/work/curl/curl/tests/../docs/libcurl/libcurl-symbols.3:1775: referring to non-existing manpage CURLOPT_UPLOAD_FLAGS.3[LF]
test 1173...[Manpage syntax checks]

 1173: stderr FAILED:
--- log/5/check-expected	2025-01-17 20:59:19.184537642 +0000
+++ log/5/check-generated	2025-01-17 20:59:19.184537642 +0000
@@ -1 +1,2 @@
-ok[LF]
+/home/runner/work/curl/curl/tests/../docs/libcurl/curl_easy_setopt.3:667 broken reference to CURLOPT_UPLOAD_FLAGS(3)[LF]
+/home/runner/work/curl/curl/tests/../docs/libcurl/libcurl-symbols.3:1775 broken reference to CURLOPT_UPLOAD_FLAGS(3)[LF]

@dfandrich
Copy link
Contributor

dfandrich commented Jan 18, 2025 via email

@tiymat
Copy link
Contributor Author

tiymat commented Feb 9, 2025

Looks like all the checks are passing now. If there's anything else needed to get this merged (the next time the feature window is open) please let me know.

@bagder
Copy link
Member

bagder commented Feb 27, 2025

"This branch cannot be rebased due to conflicts". Can you please fix and force-push ?

@tiymat
Copy link
Contributor Author

tiymat commented Mar 3, 2025

I think the remaining test failures are unrelated to my changes.

@tiymat
Copy link
Contributor Author

tiymat commented Mar 4, 2025

Thanks for the review! I've pushed a patch that addresses your comments.

@bagder
Copy link
Member

bagder commented Mar 4, 2025

The dist / maketgz-and-verify-in-tree (pull_request) failure is really weird.

runtests.pl exits with this:

Cannot open ./data/DISABLED, exiting

@bagder bagder closed this in 6758aa7 Mar 4, 2025
@bagder
Copy link
Member

bagder commented Mar 4, 2025

Thanks a lot for your hard work and patience with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmdline tool feature-window A merge of this requires an open feature window libcurl API tests
Development

Successfully merging this pull request may close these issues.

4 participants