Re: gnupload improvements

[Ralf Wildenhues]

Hello Sergey,

* Sergey Poznyakoff wrote on Sat, Jan 24, 2009 at 03:00:44PM CET:
> 
> I'd like to propose several improvements in gnupload. The patch
> is attached.

Thanks for your work on this, and sorry for the delay.  FWIW, it would
have been easier to review if you had split this into several patches.
But anyway:

> 3. Support for removal and creating symlinks.
> 
> Three command line options are introduced for this purpose. The
> --rmsymlink option causes removal of the symlinks listed in the command
> line. The --symlink option creates symbolic links. It takes even number
> of arguments, e.g.:
> 
>   --symlink a b c d
> 
> will create two symbolic links: a -> b and c -> d.

This API looks rather unusual.  I'd have expected something like a comma
delimiter, not more than one argument, or at least not an arbitrary
(even) number of arguments, and an ordering akin to ln, i.e.,
  --symlink=b,a --symlink d c

or so (the latter avoids the need for comma escaping).  I think in your
actual patch, you are linking b -> a though, so that one is ok.

Also, why is --rmsymlink not --delete?  (This is me wondering about the
file format, not a question about your patch.)

> Finally, the --symlink-re option allows to create symbolic links along
> with uploading new releases. For example, the following command:

Can we make that --symlink-regex, to avoid abbreviating?

>   gnupload --to ... --symlink-re foo-1.2.tar.gz
> 
> uploads the file foo-1.2.tar.gz and creates a symbolic link:
> foo-latest.tar.gz -> foo-1.2.tar.gz. As an optional argument, this
> option allows to specify a sed expression to transform file names into
> link names.
> 
> 4. Any number of operations can be specified in a single invocation,
> e.g.:
> 
> gnupload --to alpha.gnu.org:tar \
>          --delete tar-1.20.90.tar.gz tar-1.20.90.tar.bz2 \
>          --rmsymlink tar-latest.tar.gz tar-latest.tar.gz2 \
>          -- tar-1.20.91.tar.gz
> 
> (double-dash in this case is needed to separate files to upload
> from --rmsymlink arguments).   

Do we want to make guarantees about the ordering in which actions are
taken?

> 5. Debugging features:
> 
> The --dry-run option causes gnupload to print what it would have done,
> without actually uploading anything. It also prints the contents of the
> created directive file(s).

Nice.

> The --to command option allows for the argument in form DIR:SUBDIR,
> where DIR is an absolute directory name (--to /tmp:tar). This instructs
> gnupload to copy the created files to /tmp. This is useful for debugging
> the script itself and for debugging the software implementing automated
> upload procedures.

OK.


> --- orig/gnupload     2008-12-28 15:44:04.000000000 +0200
> +++ lib/gnupload      2009-01-24 15:54:33.000000000 +0200

> @@ -24,97 +24,192 @@ set -e
>  
>  GPG='gpg --batch --no-tty'
>  to=
> -delete=false
> +DRY_RUN=
> +SYMLINK_FILES=
> +DELETE_FILES=
> +DELETE_SYMLINKS=
> +COLLECT_VAR=
> +DBG=

Please, no upper-case variables.  This isn't Fortran, and most reserved
variables are upper-case.

> +Commands:
> +  --delete                 delete FILES from destination
> +  --symlink                create symbolic links
> +  --rmsymlink              remove symbolic links 

BTW, you can avoid adding trailing white space with something like
  chmod +x .git/hooks/pre-commit

in the git tree, then 'git commit' will warn.  It does when I try to
apply your patch to my tree (several instances).

> +  --                       treat the remaining arguments as files to upload
> +  
>  Options:
>    --help                   print this help text and exit
>    --to DEST                specify one destination for FILES
>                             (multiple --to options are allowed)
>    --user NAME              sign with key NAME
> -  --delete                 delete FILES from destination instead of uploading
> +  --symlink-re[=SED-EXPR]  use SED-EXPR to create symbolic links
> +  --dry-run                do nothing, show everything

  s/everything/what would be done/  no?

>    --version                output version information and exit
>  
> +If --symlink-re without SED-EXPR is given, symlink target name is
> +created by replacing version information with the word \`-latest',
> +e.g.:

If --symlink-re is given without SED-EXPR, then the link target name
is created by replacing the version information with \`-latest', e.g.:

> +  foo-1.3.4.tar.gz -> foo-latest.tar.gz
> +

> -Deletion only works for ftp.gnu.org and alpha.gnu.org (using the
> -archive: directive).  Otherwise it is a no-op.  Deleting a file foo also
> -deletes foo.sig; do not specify the .sig explicitly.
> -
> -Simple single-target single-file examples:
> -  gnupload --to alpha.gnu.org:automake automake-1.8.2b.tar.gz
> -  gnupload --to ftp.gnu.org:automake automake-1.8.3.tar.gz
> -  gnupload --to alpha.gnu.org:automake --delete automake-oops.tar.gz
> +If the file .gnupload exists in the current working directory, its contents
> +is prepended to the actual command line options.  Use this to keep your

s/is/are/

> +defaults.  Comments (#) and empty lines in .gnupload are allowed.
> +

>  Report bugs to <address@hidden>.
>  Send patches to <address@hidden>."
>  
> +# Read local configuration file
> +if [ -r .gnupload ]; then

Let's stick to writing `[ ... ]' as `test ...', for consistency with
most of autotools.

> +  echo "$0: Reading configuration file .gnupload"
> +  eval set -- "`sed 's/#.*$//;/^$/d' .gnupload | tr '\n' ' '` $*"

A couple of portability nits:
- 'set x ...; shift' 
- \"address@hidden" instead of $*
- not all tr implementations grok \n, and what about \r?
  I'd probably use
    tr '\012\015' '  '
  as non-ASCII systems can safely be ignored.

> +fi
> +    
>  while test -n "$1"; do
>    case $1 in
> -    --delete)
> -      delete=true
> -      shift
> -      ;;
> -    --help)
> -      echo "$usage"
> -      exit $?
> -      ;;
> -    --to)
> -      if test -z "$2"; then
> -     echo "$0: Missing argument for --to" 1>&2
> -        exit 1
> -      else
> -        to="$to $2"
> -        shift 2
> -      fi
> +  -*) COLLECT_VAR=
> +      case $1 in
> +        --help)

There is no need to nest case statements here, no?

> +        --symlink-re=*)
> +          SYMLINK_EXPR=${1##--symlink=}

This isn't portable.  OTOH, allowing --OPT=ARG would be nice for
other arguments like --to and --user, too.  Why not go with the
Autoconf code?

loop over args ...

  # If the previous option needs an argument, assign it.
  if test -n "$prev"; then
    eval $prev=\$option
    prev=
    continue
  fi

  case $option in
  *=*)  optarg=`expr "X$option" : '[^=]*=\(.*\)'` ;;
  *)    optarg=yes ;;
  esac


> +          shift
> +          ;;
> +        --symlink-re)
> +          SYMLINK_EXPR='s|-[0-9][0-9\.]*\(-[0-9][0-9]*\)\?\.|-latest.|'

\? is not portable sed, but \{0,1\} is pretty portable.

> +          shift
> +          ;;
> +        --symlink)
> +          COLLECT_VAR=SYMLINK_FILES
> +          shift
> +          ;;

> +  *)  if test -z "$COLLECT_VAR"; then
> +        break
>        else
> -        GPG="$GPG --local-user $2"
> -        shift 2
> +        eval $COLLECT_VAR=\"\$$COLLECT_VAR $1\"

Even if not strictly necessary here, I'd put the string to be eval'ed in
double quotes.

> -if test $# = 0; then
> -  echo "$0: No file to upload or delete" 1>&2
> +dprint() {

Let's stick with GNU style indentation:

dprint ()
{
  ...

> +  echo "Running $*..."
> +}
> +

> +if test -n "$SYMLINK_FILES"; then
> +  if test -n "`echo "$SYMLINK_FILES" | sed 's/[^ ]//g;s/  //g'`"; then

Nested double- and backquotes are not portable (but assigning the result
of a backquote substitution to a variable is, without further outer
double quotes).

> +    echo "$0: Odd number of symlink arguments" >&2
> +    exit 1
> +  fi
> +fi  
> +
> +if test $# = 0; then
> +  if test -z "${SYMLINK_FILES}${DELETE_FILES}${DELETE_SYMLINKS}"; then
> +    echo "$0: No file to upload" 1>&2
> +    exit 1
> +  fi
[...]

If you agree with my comments, and answer the implied questions, I can
make those changes; if you want to resubmit the patch, even better.  :-)

I do with we had some testsuite exposure for gnupload though, this has
grown so complicated that bugs are likely.  Is there test upload space
on some of these hosts?

Thanks,
Ralf