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

Implementing ETag compare options for the curl tool #4543

Closed
wants to merge 1 commit into from

Conversation

@s-3ntinel
Copy link

s-3ntinel commented Oct 30, 2019

Implementing code for a feature request #4277

Regarding saving an ETag, I'm gonna be saving only ETag to a file not URLs with it.
As far as comparing ETag from a file to the one in the header of a request, i don't really know what should be a successful output of a request with compare.
@paulehoffman of @bagder do you have any ideas about the functionality of compare?

Will add more stuff to help and hugehelp later.

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Oct 30, 2019

Sounds like you're off to a good start!

I would encourage you to write up a test case or two really early on so that you can work on making sure those pass, as then you get the functionality there at the same time as you get a test or two added. The compare operation needs to pass in the given ETag in the HTTP request so that it will be made conditional. I would recommend you make yourself familiar with RFC7232 section 3.2.

As you see, the CI builds fail on test 1139. If you run that locally, check the tests/log/stderr* and stdout* files after the failed test to see what it doesn't like.

@bagder bagder changed the title Implementing Feature #4277 Implementing ETag compare options for the curl tool Nov 2, 2019
@s-3ntinel s-3ntinel force-pushed the s-3ntinel:feature-#4277 branch 3 times, most recently from 201d4e6 to 700f763 Nov 4, 2019
@s-3ntinel s-3ntinel marked this pull request as ready for review Nov 4, 2019
@s-3ntinel

This comment has been minimized.

Copy link
Author

s-3ntinel commented Nov 7, 2019

can someone look at this?

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 7, 2019

tool_cb_hdr.obj : error LNK2019: unresolved external symbol Curl_strtok_r referenced in function tool_header_cb
..\builds\libcurl-vc14-x64-debug-dll-ssl-dll-ipv6-sspi\bin\curl.exe : fatal error LNK1120: 1 unresolved externals

This happens because you use Curl_strtok_r from the tool code, while the function is a private one in the library...

docs/cmdline-opts/etag-compare.d Outdated Show resolved Hide resolved
docs/cmdline-opts/etag-compare.d Outdated Show resolved Hide resolved
docs/cmdline-opts/etag-compare.d Outdated Show resolved Hide resolved
docs/cmdline-opts/etag-save.d Outdated Show resolved Hide resolved
src/tool_cb_hdr.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
@s-3ntinel s-3ntinel force-pushed the s-3ntinel:feature-#4277 branch from 700f763 to d6c4b88 Nov 9, 2019
@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Nov 9, 2019

This pull request introduces 1 alert when merging d6c4b88 into cba52e2 - view on LGTM.com

new alerts:

  • 1 for No space for zero terminator
@s-3ntinel s-3ntinel force-pushed the s-3ntinel:feature-#4277 branch 4 times, most recently from 816bf28 to 00ce754 Nov 9, 2019
@s-3ntinel

This comment has been minimized.

Copy link
Author

s-3ntinel commented Nov 10, 2019

can look at this again 😄

Copy link
Member

bagder left a comment

Awesome. Please consider adding a test case or two for next round. They will help to verify that things work as intended, and that there are no leaks etc.

src/tool_cb_hdr.c Outdated Show resolved Hide resolved
src/tool_cb_hdr.c Outdated Show resolved Hide resolved
src/tool_cb_hdr.c Outdated Show resolved Hide resolved
src/tool_cb_hdr.c Outdated Show resolved Hide resolved
src/tool_cb_hdr.c Outdated Show resolved Hide resolved
src/tool_operate.c Show resolved Hide resolved
src/tool_operate.c Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
src/tool_operate.c Outdated Show resolved Hide resolved
@s-3ntinel s-3ntinel force-pushed the s-3ntinel:feature-#4277 branch 2 times, most recently from a75a8e1 to face2b3 Nov 11, 2019
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 12, 2019

I love the test cases, but I miss one like test case 341 but with an existing file file so that we get an actual Etag match case tested. And then maybe one with a compare that fails to match?

The travis builds find memory leaks.

The appveyor builds warn on this:

C:\projects\curl\src\tool_cb_hdr.c(142,33): warning C4244: '=': conversion from '__int64' to 'long', possible loss of data [C:\projects\curl\src\curl.vcxproj]
@s-3ntinel s-3ntinel force-pushed the s-3ntinel:feature-#4277 branch from face2b3 to bb9fe41 Nov 13, 2019
removing unnecessary custom files, putting header processing to header callback function

Adding new OutStruct for saving ETag to a file, and implementing function for saving

adding compare functionality in tool_operate, and waiting for response header callback in tool_cb_hdr

testing compare func

fixing minor issues with writing, and constructin If-None-Match etag header with different functions

adding tmp man pages, + fixing string formatting in etag-compare

fix typos in man option definition, import library for strtok_r

another fixes to satisfy builds

create two new curl tool options to work with etag

rewriting man pages, fixing issues mentioned in review

fix sscanf width restriction

lgtm complaint

init local variable, alloc memory for it

code pr quality fix

fixes after review, adding 2 tests for --etag-save and --etag-compare

type fix

add new test, fix build errors and leaks

type error
@s-3ntinel s-3ntinel force-pushed the s-3ntinel:feature-#4277 branch from bb9fe41 to 6a0d7be Nov 13, 2019
@s-3ntinel

This comment has been minimized.

Copy link
Author

s-3ntinel commented Nov 14, 2019

How about now? I see some failed tests but i think its a fault build

@bagder
bagder approved these changes Nov 15, 2019
@bagder bagder closed this in 18e5cb7 Nov 28, 2019
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 28, 2019

Thanks!

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Dec 2, 2019

@s-3ntinel any particular reason why both options can't be used at once?

It seems like the perfect use case to run in a cronjob, like:

$ curl https://curl.haxx.se/ --etag-compare moo -v --etag-save moo -o file

Only update once the etag changes, and if it changes save it again to the output file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.