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

error out if --next is used without a prior URL #10782

Closed
wants to merge 3 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Mar 16, 2023

No description provided.

kadler pushed a commit to kadler/curl that referenced this pull request Mar 17, 2023
@bagder bagder closed this in e2452cf Mar 17, 2023
@bagder bagder deleted the bagder/next-wo-url branch March 17, 2023 13:07
@jidanni
Copy link
Contributor

jidanni commented Mar 17, 2023

All I know is I see "error out if --next is used without a prior URL".

It is really hard to tell using my small computer screens what is happening in the code.

So I'll just have to rely on the titles.

If you are making --next more forgiving, that's good!
If you are making --next more strict, that's bad.

If the latter is the case, then I was going to say the following, which maybe isn't now necessary:


I am saying just like perl -we 'for(1){next; next; next;}' does it produce an error? No. Not even a warning.

So --next should just do the job that it says it does on the command
line: clearing local options.

Currently it has an unnecessary internal check that there must be a URL
following it somewhere.

Please eliminate this unnecessary check and resultant error.

Now you want to even add another unnecessary check: to make sure there
is a URL before a next somewhere too.

Please don't add that unnecessary check either.

Just like the perl example above, the next should simply mind its own
business and do its sated job, and if the user uses it in a silly
manner, that's his own fault and his program runs a little slower,
that's the price he should pay.

So what about

$ curl -w 'moo %{http_code}\n' \
	--next ...

Well, the --next should simply wipe out the -w,
exactly like it says it should in the man page.

Anyway, let's take a look at a real life config file producer that runs
1000 times:

for ( @{ $p->{query}->{usercontribs} } ) {
    my $ur;
    my $fn = $_->{title};
    for ($fn) {
        s/ /_/g;
        $ur = uri_escape_utf8($_);
        s/^File:// or die;
        s/$/$ENV{SUF}/;
    }
    print <<EOF;    #next BEFORE, not after, the group:

next
write-out = "%{http_code} %{url}\\n"
url = "$ENV{TMPL}$ur"
time-cond = "$fn"
output = "$fn"
EOF
}

Here we see the user already learned he needs to put the 'next' before,
not after.

Now that won't even be good enough. Now he will need to double the size
of his code or something, because the next can only be written
1000-1=999 times.

@bagder
Copy link
Member Author

bagder commented Mar 17, 2023

All I know is I see "error out if --next is used without a prior URL".

Then you made a weird command line and curl tells you about it.

If you are making --next more strict, that's bad.

I disagree. Before this change, curl accepted the syntax but the outcome was not clear or even understandable - you reported a bug that was a direct result of that internal confusion. By allowing wrong syntax and just silently trying to survive anyway, we make the behavior harder to understand and less deterministic.

the user already learned he needs to put the 'next' before, not after.

That user does not use --next as it is documented.

Now he will need to double the size of his code or something, because the next can only be written 1000-1=999 times.

I don't understand. Why can it only be written 999 times?

@jidanni
Copy link
Contributor

jidanni commented Mar 17, 2023

Then you made a weird command line and curl tells you about it.

I was talking about the GitHub title.

Anyway, for a large batch job, in a config file, we make 1000 of these, (here's two):

write-out = ...
url = ...
time-cond = ...
output = ...
next

write-out = ...
url = ...
time-cond = ...
output = ...
next

That already caused an error.
Good thing we can put the 'next' at the front instead of the back:

next
write-out = ...
url = ...
time-cond = ...
output = ...

next
write-out = ...
url = ...
time-cond = ...
output = ...

and the error goes away! But today, you are proposing to also make that
raise an error.

That means that there is no longer to make 1000 sets each with five
lines. The 'next' on the first one or very last one needs to be removed.
All for no real gain. Nor does it break any rule on the man page.

Sure, of my 1000 sets, the very first or the very last next serves no
purpose, but so what.

It is like

perl -awe '@F=(1,2,3,);'

Oh boy, there is a wasted comma! Error: not at all.
Semi-colon could also be removed. But OK if it stays.

@stoat1
Copy link

stoat1 commented May 31, 2023

Am I mistaken or this closed PR somehow found its way into master?

curl -: http://example.com
curl: missing URL before --next
curl: option -:: is badly used here
curl: try 'curl --help' for more information

curl --version
curl 8.0.1 (x86_64-pc-linux-gnu) libcurl/8.0.1 OpenSSL/3.0.8 zlib/1.2.13 brotli/1.0.9 zstd/1.5.5 libidn2/2.3.4 libpsl/0.21.2 (+libidn2/2.3.4) libssh2/1.10.0 nghttp2/1.52.0
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS brotli GSS-API HSTS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz NTLM NTLM_WB PSL SPNEGO SSL threadsafe TLS-SRP UnixSockets zstd

@stoat1
Copy link

stoat1 commented Jun 1, 2023

One argument for allowing the redundant placement of -:: it makes the command syntax composable. Therefore, it allows for programmatically generated argument lists — the use case in which -: brings the most value in terms of optimizations that come with it: persistent connections and --parallel.

for x in foo bar baz
do
  echo -: http://example.com --url-query x=$x
  #  echo http://example.com --url-query x=$x -:
done |

  xargs -n 8 curl

@bagder
Copy link
Member Author

bagder commented Jun 1, 2023

Am I mistaken or this closed PR somehow found its way into master?

When it says "@bagder closed this in e2452cf on Mar 17" like this:

Screenshot 2023-06-01 at 13-47-27 error out if --next is used without a prior URL by bagder · Pull Request #10782 · curl_curl

... it means it was merged. GitHub just refuses to show it as such. So yes, it has been shipped since 8.0.0.

bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
@lindhe
Copy link

lindhe commented Sep 26, 2024

I just want to mention a work-around I found, for anyone else who want to skip the first URL: just put file:///dev/null there! 🙃

My use-case for doing this is that I have a loop that constructs a curl command (adding one request per item in a list i iterate over). Instead of conditionally adding --next (depending on if it's the first/last iteration), which would require additional logic in my script, I just add file:///dev/null as the "prior URL" and curl will harmlessly (I hope) call that before proceeding.

The end result looks like this: curl file:///dev/null --next https://example.com --next example.com

Works like a charm! 👍

@stoat1
Copy link

stoat1 commented Sep 27, 2024

Next thing you know cURL no longer allows file:///dev/null because the outcome would be "not clear or even understandable" for certain users 🙃

@bagder
Copy link
Member Author

bagder commented Sep 27, 2024

Sarcasm all you like. We make our best.

@lindhe
Copy link

lindhe commented Sep 28, 2024

You are a true open source rock star, Daniel. Thank you (and the team) for your efforts with curl! ❤️

This was a breaking change in a major release, so absolutely no biggie as far as I'm concerned. And it was pretty easy for me to find this PR by checking the release notes, so I was able to identify the exact change that I was affected by. That is all great.

But I find no details on the reason for this change. Is that anywhere to find?

I think that sometimes people can feel frustrated when a change is introduced if they do not understand the reason for it. If the PR description or commits had a brief summary of why a change is introduced, maybe it would help people understand and reduce irritation.

Unless it is too much effort for you, perhaps that could be an improvement for future releases.

@bagder
Copy link
Member Author

bagder commented Sep 28, 2024

But I find no details on the reason for this change. Is that anywhere to find?

I have explained them earlier in this PR:

Before this change, curl accepted the syntax but the outcome was not clear or even understandable. By allowing wrong syntax and just silently trying to survive anyway, we make the behavior harder to understand and less deterministic.

I should have done a better job and explained that already in the PR description, which is my bad.

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.

4 participants