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 <[email protected]>. > Send patches to <[email protected]>." > > +# 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' - \"$...@\" 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
