self-update: better network error handling

[guihkx]

By using curl's -f option, we improve the self-update mechanism by not overwriting the fisher script if the HTTP status code given to us by Github is higher than 400.

Additionally, we make sure that the exit code of the curl process must be always zero for the self-update to succeed.

The -S option, when used with -s, allows us to get an error message from curl when it fails.

A few examples:

Fixes #578

[jorgebucaran]

@guihkx By using curl's -f option, we improve the self-update mechanism by not overwriting the fisher script if the HTTP status code given to us by Github is higher than 400.

Why do we need to duplicate error handling -- where do we fail here?

fisher/fisher.fish

Lines 162 to 167 in 994506e

	case "" $fisher_version
	command rm -f $file.
	if test -z "$next_version"
	echo "fisher: cannot update fisher -- are you offline?" >&2
	return 1
	end

[guihkx]

I really had considered removing that check, but curl's -f option only fails on HTTP codes equal or higher than 400.

But what if something goes really wrong with Github servers and they give us a 3xx code? We're currently not using curl's -L option to follow the Location header. Or even a 200 OK response, but with wrong response body (e.g. empty or with some generic error)?

In this case I think it's better be safe than sorry. Also, that's an useful check in case awk fails (for whatever reason).

Thoughts?

[jorgebucaran]

@guihkx Gotcha. Could there be a way to get whatever curl gives us, and use awk to validate the file avoiding handling errors twice? If you could list up any of the possible error responses, I could provide the correct awk incantations.

[guihkx]

I just learned that we can use curl to give us the HTTP status code directly to stdout. For instance:

$ curl -sw '%{http_code}' 'https://raw.githubusercontent.com/jorgebucaran/fisher/master/fisher.fish' -o $file.

If the output is different than 200, we simply abort and show a generic error with the HTTP status code we got from Github.

I mean, I think it's safe to assume that if we get a HTTP 200 OK code, the content is "trustworthy".

Then that second check could be fully removed.

[jorgebucaran]

@guihkx Interesting, where would that leave us?

[guihkx]

I just pushed the modifications I did.

It only does a single check now: unless we get a 200 OK, the self-update will abort with an error:

I personally think the changes are sane, but let me know if you think it's "too much".

By the way, I'm not that familiar with the fish syntax, so any optimizations you can think of are very much welcome! :)

[jorgebucaran]

@guihkx Whoops, I think I miscommunicated here. I'm trying to both eliminate duplication (#580 (comment)) and keep the code simple. Now, there's more code than in your original PR. 😅

If possible, I'd like to dump whatever curl returns in a temporary file, then use awk to validate the content.

[guihkx]

Ohhhh, gotcha now! 😆

But when you say "validate the content", are you intending to check if output given by curl is an actual, valid, fish script? If that's the case, I think it sounds too complicated and I have no idea how can we do that properly. Maybe if fish has a tool for validating the syntax of their scripts or something like that...?

[jorgebucaran]

It has something "close", have you looked at fish_indent? But I don't mean let's use that or try anything too complicated.

What we are doing right now is grabbing the version number from the file with awk. Do you know how that awk script doesn't fail even when curl does? I understand that's part of the problem we have here.

[guihkx]

It has something "close", have you looked at fish_indent?

I took a quick look on its documentation right now, but I'm not sure how it would be useful...? However, the fish manual pointed me to the -n flag:

       • -n or --no-execute do not execute any commands, only perform syntax checking

It seems pretty fast, too:

$ time fish -n ~/.config/fish/functions/fisher.fish &>/dev/null

________________________________________________________
Executed in    9,51 millis    fish           external 
   usr time    6,81 millis  276,00 micros    6,54 millis 
   sys time    2,27 millis   84,00 micros    2,19 millis 

$ echo $status
0

Testing with a random, non-script file (500 KiB):

$ time fish -n test.svg &>/dev/null

________________________________________________________
Executed in   24,37 millis    fish           external 
   usr time   18,34 millis  396,00 micros   17,94 millis 
   sys time    6,04 millis   58,00 micros    5,98 millis

Could that be used instead?

Do you know how that awk script doesn't fail even when curl does? I understand that's part of the problem we have here.

I think I do. As I mentioned in #578, Github went through a power outage and this is the raw response their server was giving me:

500: Internal Server Error

Running that output on awk:

$ echo '500: Internal Server Error' | awk '{ print $4 } { exit }'
Error

In order for the self-update to fail, the output of the command above would have to be an empty string, right?

[jorgebucaran]

@guihkx Hmm, then this might just be everything we need:

set -l next_version (command awk '$4 ~ /^[0-9]+\.[0-9]+\.[0-9]+$/ { print v=$4 } { exit !v }' <$file.)

Could you check?

UPDATE: Fixed regex!

[guihkx]

I was just about to suggest using regex, but I had no idea on how to do that with awk. x)

Does fisher follow semantic versioning? For instance that check would fail if you released version 3.10.1.

[jorgebucaran]

@guihkx Yes, whoops, my bad. This should work:

awk '$4 ~ /^[0-9]+\.[0-9]+\.[0-9]+$/ { print v=$4 } { exit !v }'

[guihkx]

Just tested it, looks good!

Don't forget to squash this whole mess before merging. 😅

Thanks a bunch!

[jorgebucaran]

Thank you, @guihkx ! 3.2.12 is now officially out! All fishfolk rejoice! 🦐