[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: gnupload improvements
From: |
Ralf Wildenhues |
Subject: |
Re: gnupload improvements |
Date: |
Mon, 16 Feb 2009 20:18:26 +0100 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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
- Re: gnupload improvements,
Ralf Wildenhues <=