Re: Patch to tidy up wget use, and add return=4 when 404's a…

On Friday 07 March 2008, Philip Hands wrote:
> Frans Pop wrote:
> > Or maybe use a switch to toggle between "continuation" and "full"
> > retries. Continuation retries probably make sense for larger files
> > (Packages comes to mind) for which the integrity can be checked
> > (md5sum).
> > Given the caveats in the wget manpage for -c, it may be safer to not do
> > continuation retries at all, but just do a full retry in other cases
> > (like fetching preseed files).
>
> OK, I've done -c as an option to fetch-url that adds a -c option to wget
> for the 3 retries that it tries if the initial wget fails. I've also
> made preseed_fetch accept options and pass them onto fetch-url, so you
> can use -c in preseed scripts (for downloading udebs or debs, say)

Hmm. That is not what I had in mind. Also, you now *always* run with -c if
it is passed (even on the first try) and I don't think that is desirable
(and even very dangerous).

Let me show what I'm thinking in plain text^Wcode.
This supports two separate options -c and -r that probably should not be
used at the same time.
I've thrown in preservation of the return code for free :-)

protocol_fetch() {
    local i j RET
    [other initial stuff and proxy]

    RET=0
    for i in 1 2; do
        wget -q "$url" -O "$file" || RET=$?
        [ $RET != 0 ] || return 0

        if [ "$ALLOW_CONTINUES" = yes ]; then
            for j in 1 2 3; do
                wget -c -q "$url" -O "$file" || RET=$?
                [ $RET != 0 ] || return 0
            done
        fi
        [ "$DO_REPEAT" = yes ] || break
    done
    return $RET
}

> Here's the patch, split in two as you suggested so that the first half
> is just the moving of fetch-url into di-utils:
> http://hands.com/~phil/d-i/fetch-url.diff

Looks good. Some comments.

+++ b/packages/debian-installer-utils/fetch-url
+while true ; do

We normally don't do spaces before ;

+    case "$1" in
+        -c)
+            ALLOW_CONTINUES=yes

Current coding style prefers 4 spaces before options and then a single tab
for the code (saves an indentation level and in most cases also bytes):
+    case "$1" in
+     -c)
+        ALLOW_CONTINUES=yes

+++ b/packages/debian-installer-utils/fetch-url-methods/floppy
+    mountfloppy || true
+    touch /var/run/preseed-usedfloppy
+

Trailing spaces on the last line (also in original, but still :-).

+++ b/packages/debian-installer-utils/fetch-url-methods/http
+    [ "$ALLOW_CONTINUES" = "yes" ] && local WGET_C="-c"

This breaks when run with 'set -e'.

+++ b/packages/preseed/preseed_fetch
+# eat options starting with a -, so we can pass them on
+while expr "$1" : "-" >/dev/null ; do

Doesn't this test for -'s anywhere in $1, not only at the start?
And space before ; again.

> and the second bit is the wget404 stuff:
> http://hands.com/~phil/d-i/wget404.diff

Similar minor coding style issues and main patch will need rewrite depending
on final version of protocol_fetch.

Cheers,
FJP