Re: [Quilt-dev] [patch 2/5] Return 2 (instead of 1) when there is nothing to do

[Jean Delvare]

Le mercredi 19 décembre 2012 à 15:35 +0100, address@hidden a
écrit :
> pièce jointe document texte brut (return2)
> Description: Return 2 (instead of 1) when there is nothing to do
>  This is mandatory to differenciate "error" and "everything's done".

Spelling: differentiate.

>  .
>  This really ease the scripting around quilt, eg in patchsys-quilt.mk 
>  .
>  It used to works this way in quilt 0.33, and was changed for some
>  reason upstream.

This was changed in version 0.43 by this commit:

commit 188c7dac15a72c437c47664d3162b9f13844fe88
Author: John Mark Vandenberg <address@hidden>
Date:   Thu Jan 19 08:02:05 2006 +0000

    - Move patch parameter checks into patchfns.in, adding quotes
      around all patch parameters, and reporting 'No patches in series'
    - quilt/annonate.in: Fix case where no patches have been applied.

The change in returned value is an unintended side effect of the move of
common code to patchfns.

> Bug-Debian: http://bugs.debian.org/358792
> Forwarded: Submitted 2012-12-19
> 
> ---
>  quilt/push.in             |    2 +-
>  quilt/scripts/patchfns.in |    8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> --- a/quilt/push.in
> +++ b/quilt/push.in
> @@ -362,7 +362,7 @@ else
>       [ -z "$opt_all" ] && number=1
>  fi
>  
> -stop_at_patch=$(find_unapplied_patch "$stop_at_patch") || exit 1
> +stop_at_patch=$(find_unapplied_patch "$stop_at_patch") || exit $?

Just "exit" will also return $?, and this would be consistent with how
the "next" command does it.

>  
>  [ -z "$opt_verbose" ] && silent_unless_verbose=-s
>  [ -n "$opt_force" ] && opt_leave_rejects=1
> --- a/quilt/scripts/patchfns.in
> +++ b/quilt/scripts/patchfns.in
> @@ -477,7 +477,7 @@ find_last_patch()
>               else
>                       printf $"No series file found\n" >&2
>               fi
> -             return 1
> +             return 2
>       fi
>  
>       echo "$patch"

This change doesn't have any effect in practice. The only caller of
find_last_patch() is "quilt mail" and it overwrites the returned value.
Plus this would make find_first_patch and find_last_patch diverge for no
good reason.

> @@ -582,7 +582,7 @@ find_unapplied_patch()
>               then
>                       printf $"Patch %s is currently applied\n" \
>                               "$(print_patch $patch)" >&2
> -                             return 1
> +                             return 2
>               fi
>               echo "$patch"
>       else
> @@ -592,13 +592,13 @@ find_unapplied_patch()
>               then
>                       patch_after "$start"
>               else
> -                     find_first_patch || return 1
> +                     find_first_patch || return 2
>               fi
>               if [ $? -ne 0 ]
>               then
>                       printf $"File series fully applied, ends at patch %s\n" 
> \
>                               "$(print_patch $start)" >&2
> -                     return 1
> +                     return 2
>               fi
>       fi
>  }

I would love if this patch would also add test cases for all conditions
which are supposed to return 2. This would not only guarantee that such
a regression isn't reintroduced later, but this would also ensure that
the fix itself is complete. Unfortunately the testing script doesn't
support for this yet, but I'm working on it.

It would also be great to update the manual page to document these
cases, as it currently lacks an EXIT STATUS section.

-- 
Jean Delvare
Suse L3