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

"spaceship" progress bar breaks at >=150 columns #4849

Closed
apottere opened this issue Jan 24, 2020 · 12 comments
Closed

"spaceship" progress bar breaks at >=150 columns #4849

apottere opened this issue Jan 24, 2020 · 12 comments
Labels

Comments

@apottere
Copy link

@apottere apottere commented Jan 24, 2020

I did this

$ echo $COLUMNS
149
$ /usr/local/opt/curl/bin/curl --http0.9 --progress-bar localhost:1234 >/dev/null
^C                                                                                     #          #         #         #   -=O=-

$ echo $COLUMNS
150
$ /usr/local/opt/curl/bin/curl --http0.9 --progress-bar localhost:1234 >/dev/null
                                         -=O=-                                                                                            #     #  # #
                                          -=O=-                                                                                              #   #  ##
                                           -=O=-                                                                                               #  # ##
                                            -=O=-                                                                                               #  ###
                                             -=O=-                                                                                                ####
                                              -=O=-                                                                                               ####
                                               -=O=-                                                                                             #  ##
                                                -=O=-                                                                                           #  ###
                                                 -=O=-                                                                                        #   # ##
                                                  -=O=-                                                                                     #   #  # #
^C                            #         #          #          #              -=O=-

I expected the following

Using the progress bar with a 149-column terminal works just fine, increasing the terminal size to anything 150 columns and over (tested up to 172) goes 💥 when the hashes hit the side of the "screen"

curl/libcurl version

both system and homebrew versions are affected:

$ curl --version
curl 7.64.1 (x86_64-apple-darwin19.0) libcurl/7.64.1 (SecureTransport) LibreSSL/2.8.3 zlib/1.2.11 nghttp2/1.39.2
Release-Date: 2019-03-27
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS GSS-API HTTP2 HTTPS-proxy IPv6 Kerberos Largefile libz MultiSSL NTLM NTLM_WB SPNEGO SSL UnixSockets
$ /usr/local/opt/curl/bin/curl --version
curl 7.68.0 (x86_64-apple-darwin19.2.0) libcurl/7.68.0 SecureTransport zlib/1.2.11
Release-Date: 2020-01-08
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile libz NTLM NTLM_WB SSL UnixSockets

operating system

OSX Catalina

@apottere
Copy link
Author

@apottere apottere commented Jan 24, 2020

As a workaround till this is fixed, is there a way to limit the columns curl will use in larger terminals? I tried overriding COLUMNS=10, which should have worked according to #2242 (comment), but it's still using the full width of the terminal

Edit: Nevermind, it looks like it just ignores values that are too small, using COLUMNS=80 worked.

@emilengler
Copy link
Contributor

@emilengler emilengler commented Jan 24, 2020

Have you changed your column size while curl was running?

@apottere
Copy link
Author

@apottere apottere commented Jan 24, 2020

No, it was changed inbetween invocations and static while curl was running.

@emilengler
Copy link
Contributor

@emilengler emilengler commented Jan 24, 2020

I also have the issue when I change the window size I don't see any problems with 150

@bagder bagder added the cmdline tool label Jan 24, 2020
@bagder
Copy link
Member

@bagder bagder commented Jan 24, 2020

I can't spot anything in the code that has any particular width restriction below 256. 256 is the widest the code will go.

Is there's a chance your terminal does something odd?

@apottere
Copy link
Author

@apottere apottere commented Jan 24, 2020

It's possible, but I tested it in iTerm as well as Apple Terminal with the same behavior, as well as with my personal bashrc disabled.

@apottere
Copy link
Author

@apottere apottere commented Jan 25, 2020

Just tried it in WSL Ubuntu and it's broken with 120 columns:

$ curl --progress-bar localhost:9000 >/dev/null
                                          -=O=-                                                                 #   # ##
                                            -=O=-                                                                  # ###
                                             -=O=-                                                                  ####
                                               -=O=-                                                                # ##
                                                 -=O=-                                                           #  # ##
                                                  -=O=-                                                        #   # # #
                                                                                      -=O=-                     #   # ##
                                                                                    -=O=-                          # ###
                                                                                   -=O=-                            ####
                                                                                 -=O=-                              # ##
                                                                               -=O=-                             #  # ##
                                                                              -=O=-                            #   # # #
^C                                                                   -=O=-              #       #     #     #           
@apottere
Copy link
Author

@apottere apottere commented Jan 25, 2020

Manually setting the columns to 1 less than the actual width fixes it:

$ echo $COLUMNS
120                                                                                                                     
$ COLUMNS=120 curl --progress-bar localhost:9000 >/dev/null
                                          -=O=-                                                                 #   # ##
                                            -=O=-                                                                  # ###
                                             -=O=-                                                                  ####
                                               -=O=-                                                                # ##
                                                 -=O=-                                                           #  # ##
                                                  -=O=-                                                        #   # # #
^C                                                         #       -#O=-     #        #                                 
$ COLUMNS=119 curl --progress-bar localhost:9000 >/dev/null
^C                               #        #        #        #             -=O=-                                         

Proof that there are actually 120 columns in my terminal, the following doesn't wrap:

$ for i in $(seq 1 120); do echo -n 'x'; done; echo                                                       
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
@apottere
Copy link
Author

@apottere apottere commented Jan 25, 2020

After some more investigation: Ubuntu WSL seems to break at 120 columns, but not at 119 or 121 columns, consistently. It also seems to break >140 columns.

@bagder
Copy link
Member

@bagder bagder commented Jan 25, 2020

I can reproduce it at 120. It's a math precision error. PR is pending...

@bagder
Copy link
Member

@bagder bagder commented Jan 25, 2020

the problem

When the full width is 120. We use width - 2 (118) to put the hashtags in. We store that in check

The sinus table has a max value of 9999. We divide 9999 with (10000 / check). 10000/118 is 84.

9999 / 84 == 119, which is larger than the 118 which is supposed to be the maximum. This happens because we loose precision in fixed-point math.

the fix

We can fix this issue by increasing the precision in the formula. We make the sinus values multiplied with another 100 and we divide with a larger number.

Now the largest sinus value is 999999 and we divide it with (1000000/check) == 8474.

999999 / 8474 == 118.

bagder added a commit that referenced this issue Jan 25, 2020
The fixed-point math made us lose precision and thus a too high index
value could be used for outputting the hashtags which could overwrite
the newline.

The fix increases the precision in the sinus table and position math.

Reported-by: Andrew Potter
Fixes #4849
@apottere
Copy link
Author

@apottere apottere commented Jan 26, 2020

Thanks! I had a hunch it was something like that but I couldn't quite understand the code enough to figure it out. I'll update our scripts to just set the width to COLUMNS - 1 when running curl for now.

@bagder bagder closed this in 9870b80 Jan 26, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.