Patchwork [3,of,6] packaging: blindly factor out trap's cleanup function in builddeb

login
register
mail settings
Submitter via Mercurial-devel
Date Oct. 9, 2018, 9:29 p.m.
Message ID <186a840796aae2aa48ca.1539120565@cuben.lan>
Download mbox | patch
Permalink /patch/35591/
State New
Headers show

Comments

via Mercurial-devel - Oct. 9, 2018, 9:29 p.m.
# HG changeset patch
# User muxator <a.mux@inwind.it>
# Date 1539116185 -7200
#      Tue Oct 09 22:16:25 2018 +0200
# Node ID 186a840796aae2aa48ca5228fc7225779e3e0ee9
# Parent  f774dfa1595d4935879d2abbb8b4c802d8af0c65
packaging: blindly factor out trap's cleanup function in builddeb

This commit blindly extracts builddeb's trap routine in a dedicated function.
While doing so, I think two bugs are exposed, which will be addressed in the
next commits:

- single quoting around '$CLEANUP' will always evaluate to the literal
  '$CLEANUP' regardless of the variable's value. The "if" will always be true.
- the removal operation will not expand $PWD (and a variable expansion would
  need double quotes, anyways.
Yuya Nishihara - Oct. 10, 2018, 1 p.m.
On Tue, 09 Oct 2018 23:29:25 +0200, Antonio Muci via Mercurial-devel wrote:
> # HG changeset patch
> # User muxator <a.mux@inwind.it>
> # Date 1539116185 -7200
> #      Tue Oct 09 22:16:25 2018 +0200
> # Node ID 186a840796aae2aa48ca5228fc7225779e3e0ee9
> # Parent  f774dfa1595d4935879d2abbb8b4c802d8af0c65
> packaging: blindly factor out trap's cleanup function in builddeb
> 
> This commit blindly extracts builddeb's trap routine in a dedicated function.
> While doing so, I think two bugs are exposed, which will be addressed in the
> next commits:
> 
> - single quoting around '$CLEANUP' will always evaluate to the literal
>   '$CLEANUP' regardless of the variable's value. The "if" will always be true.
> - the removal operation will not expand $PWD (and a variable expansion would
>   need double quotes, anyways.
> 
> diff --git a/contrib/packaging/builddeb b/contrib/packaging/builddeb
> --- a/contrib/packaging/builddeb
> +++ b/contrib/packaging/builddeb
> @@ -13,6 +13,13 @@ CLEANUP=1
>  DISTID=`(lsb_release -is 2> /dev/null | tr '[:upper:]' '[:lower:]') || echo debian`
>  CODENAME=`lsb_release -cs 2> /dev/null || echo unknown`
>  DEBFLAGS=-b
> +
> +cleanup() {
> +    if [ '$CLEANUP' ]; then
> +        rm -r '$PWD/debian';
> +    fi
> +}
> +
>  while [ "$1" ]; do
>      case "$1" in
>      --distid )
> @@ -44,7 +51,7 @@ while [ "$1" ]; do
>      esac
>  done
>  
> -trap "if [ '$CLEANUP' ] ; then rm -r '$PWD/debian' ; fi" EXIT

Just for the record, $CLEANUP and $PWD would be expanded here, while setting
up the trap. So, yeah, PWD="'" would make it crash.

Patch

diff --git a/contrib/packaging/builddeb b/contrib/packaging/builddeb
--- a/contrib/packaging/builddeb
+++ b/contrib/packaging/builddeb
@@ -13,6 +13,13 @@  CLEANUP=1
 DISTID=`(lsb_release -is 2> /dev/null | tr '[:upper:]' '[:lower:]') || echo debian`
 CODENAME=`lsb_release -cs 2> /dev/null || echo unknown`
 DEBFLAGS=-b
+
+cleanup() {
+    if [ '$CLEANUP' ]; then
+        rm -r '$PWD/debian';
+    fi
+}
+
 while [ "$1" ]; do
     case "$1" in
     --distid )
@@ -44,7 +51,7 @@  while [ "$1" ]; do
     esac
 done
 
-trap "if [ '$CLEANUP' ] ; then rm -r '$PWD/debian' ; fi" EXIT
+trap 'cleanup' EXIT
 
 set -u