[pacman-dev] [PATCH 0/9] repo-add cleanup
Sigh. This started as a simple addition to the man page to document valid file extensions, and look what happened. The first 4 patches are a resend -- I've fixed the 2 blockers that Allan pointed out, the whitespace error I noticed, and a final tweak at Dan's suggestion to my terrible grammar in the man page. The other patches are a mix of cleanup, features, and a regression fix from an earlier patch I submitted. I wrote myself a little test suite for checking my work. It'll probably (eventually) end up as something more significant since, imo, repo-add deserves some test suite love. d Dave Reisner (9): repo-add: bashify reading of .PKGINFO file repo-add: store multi-value fields as arrays repo-add: use format_entry for all desc/depends fields repo-add.8.txt: document valid DB file extensions repo-add: enforce file extensions repo-add: fix path designation regression repo-add: avoid using ls to check for presence of files repo-add: move command invocation out of arg parsing loop repo-add: allow specifying symlink as repo file doc/repo-add.8.txt | 4 + scripts/repo-add.sh.in | 162 +++++++++++++++++++++++++---------------------- 2 files changed, 90 insertions(+), 76 deletions(-) -- 1.7.5.4
grep and sed aren't needed here, and this removes the truly ugly manipulation of IFS. The process substituion could just as well be a herestring, but it breaks vim's syntax highlighting. Style over substance, mang. Signed-off-by: Dave Reisner <d@falconindy.com> --- scripts/repo-add.sh.in | 21 +++++++-------------- 1 files changed, 7 insertions(+), 14 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 4e2e4a7..01eeb84 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -19,6 +19,8 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see <http://www.gnu.org/licenses/>. +shopt -s extglob + # gettext initialization export TEXTDOMAIN='pacman' export TEXTDOMAINDIR='@localedir@' @@ -226,19 +228,12 @@ db_write_entry() { _groups _licenses _replaces _depends _conflicts _provides _optdepends \ md5sum sha256sum pgpsig - local OLDIFS="$IFS" - # IFS (field separator) is only the newline character - IFS=" -" - # read info from the zipped package local line var val - for line in $(bsdtar -xOqf "$pkgfile" .PKGINFO | - grep -v '^#' | sed 's|\(\w*\)\s*=\s*\(.*\)|\1 \2|'); do - # bash awesomeness here- var is always one word, val is everything else - var=${line%% *} - val=${line#* } - declare $var="$val" + while read -r line; do + [[ ${line:0:1} = '#' ]] && continue + IFS=' =' read -r var val < <(printf '%s\n' "$line") + declare "$var=${val//+([[:space:]])/ }" # normalize whitespace case "$var" in group) _groups="$_groups$group\n" ;; license) _licenses="$_licenses$license\n" ;; @@ -248,9 +243,7 @@ db_write_entry() { provides) _provides="$_provides$provides\n" ;; optdepend) _optdepends="$_optdepends$optdepend\n" ;; esac - done - - IFS=$OLDIFS + done< <(bsdtar -xOqf "$pkgfile" .PKGINFO) csize=$(@SIZECMD@ "$pkgfile") -- 1.7.5.4
This ranks high on the code readability scale. The same function formats all of our data and writes to the metadata file at once. Signed-off-by: Dave Reisner <d@falconindy.com> --- scripts/repo-add.sh.in | 46 ++++++++++++++++++++++++---------------------- 1 files changed, 24 insertions(+), 22 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 8350db5..ab97952 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -292,28 +292,30 @@ db_write_entry() { # create desc entry msg2 "$(gettext "Creating '%s' db entry...")" 'desc' - echo -e "%FILENAME%\n${1##*/}\n" >>desc - echo -e "%NAME%\n$pkgname\n" >>desc - [[ -n $pkgbase ]] && echo -e "%BASE%\n$pkgbase\n" >>desc - echo -e "%VERSION%\n$pkgver\n" >>desc - [[ -n $pkgdesc ]] && echo -e "%DESC%\n$pkgdesc\n" >>desc - format_entry "GROUPS" "${_groups[@]}" >>"desc" - [[ -n $csize ]] && echo -e "%CSIZE%\n$csize\n" >>desc - [[ -n $size ]] && echo -e "%ISIZE%\n$size\n" >>desc - - # add checksums - echo -e "%MD5SUM%\n$md5sum\n" >>desc - echo -e "%SHA256SUM%\n$sha256sum\n" >>desc - - # add PGP sig - [[ -n $pgpsig ]] && echo -e "%PGPSIG%\n$pgpsig\n" >>desc - - [[ -n $url ]] && echo -e "%URL%\n$url\n" >>desc - format_entry "LICENSE" "${_licenses[@]}" >>"desc" - [[ -n $arch ]] && echo -e "%ARCH%\n$arch\n" >>desc - [[ -n $builddate ]] && echo -e "%BUILDDATE%\n$builddate\n" >>desc - [[ -n $packager ]] && echo -e "%PACKAGER%\n$packager\n" >>desc - format_entry "REPLACES" "${_replaces[@]}" >>"desc" + { + format_entry "FILENAME" "${1##*/}" + format_entry "NAME" "$pkgname" + format_entry "BASE" "$pkgbase" + format_entry "VERSION" "$pkgver" + format_entry "DESC" "$pkgdesc" + format_entry "GROUPS" "${_groups[@]}" + format_entry "CSIZE" "$csize" + format_entry "ISIZE" "$size" + + # add checksums + format_entry "MD5SUM" "$md5sum" + format_entry "SHA256SUM" "$sha256sum" + + # add PGP sig + format_entry "PGPSIG" "$pgpsig" + + format_entry "URL" "$url" + format_entry "LICENSE" "${_licenses[@]}" + format_entry "ARCH" "$arch" + format_entry "BUILDDATE" "$builddate" + format_entry "PACKAGER" "$packager" + format_entry "REPLACES" "${_replaces[@]}" + } >'desc' # create depends entry msg2 "$(gettext "Creating '%s' db entry...")" 'depends' -- 1.7.5.4
Signed-off-by: Dave Reisner <d@falconindy.com> --- doc/repo-add.8.txt | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/doc/repo-add.8.txt b/doc/repo-add.8.txt index 0196882..36855af 100644 --- a/doc/repo-add.8.txt +++ b/doc/repo-add.8.txt @@ -29,6 +29,10 @@ command line. delta specified on the command line. Multiple packages and/or delta to remove can be specified on the command line. +A package database is a tar file, optionally compressed. Valid extensions are: +``.db.tar'', ``.db.tar.gz'', ``.db.tar.bz2'', or ``.db.tar.xz''. The file does +not need to exist, but all parent directories must exist. + Common Options -------------- -- 1.7.5.4
On Wed, Jun 22, 2011 at 7:38 PM, Dave Reisner <d@falconindy.com> wrote:
Given that most people don't see this +() business on a day-to-day basis, mind extending the comment a bit and dropping 'extglob' so dumb bash people like me know what to look for in the manpage?
Fields like groups and depends should be stored as arrays. This requires rewriting our write_list_entry function to accomodate our new data type. This new function will not write to a file, but rather only format it. Signed-off-by: Dave Reisner <d@falconindy.com> --- scripts/repo-add.sh.in | 50 ++++++++++++++++++++++++----------------------- 1 files changed, 26 insertions(+), 24 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 01eeb84..8350db5 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -84,14 +84,16 @@ This is free software; see the source for copying conditions.\n\ There is NO WARRANTY, to the extent permitted by law.\n")" } -# write a list entry +# format a metadata entry # arg1 - Entry name -# arg2 - List -# arg3 - File to write to -write_list_entry() { - if [[ -n $2 ]]; then - echo "%$1%" >>$3 - echo -e $2 >>$3 +# ... - value(s) +format_entry() { + local field=$1; shift + + if [[ $1 ]]; then + printf '%%%s%%\n' "$field" + printf '%s\n' "$@" + printf '\n' fi } @@ -224,8 +226,8 @@ verify_signature() { db_write_entry() { # blank out all variables local pkgfile="$1" + local -a _groups _licenses _replaces _depends _conflicts _provides _optdepends local pkgname pkgver pkgdesc csize size url arch builddate packager \ - _groups _licenses _replaces _depends _conflicts _provides _optdepends \ md5sum sha256sum pgpsig # read info from the zipped package @@ -235,13 +237,13 @@ db_write_entry() { IFS=' =' read -r var val < <(printf '%s\n' "$line") declare "$var=${val//+([[:space:]])/ }" # normalize whitespace case "$var" in - group) _groups="$_groups$group\n" ;; - license) _licenses="$_licenses$license\n" ;; - replaces) _replaces="$_replaces$replaces\n" ;; - depend) _depends="$_depends$depend\n" ;; - conflict) _conflicts="$_conflicts$conflict\n" ;; - provides) _provides="$_provides$provides\n" ;; - optdepend) _optdepends="$_optdepends$optdepend\n" ;; + group) _groups+=("$group") ;; + license) _licenses+=("$license") ;; + replaces) _replaces+=("$replaces") ;; + depend) _depends+=("$depend") ;; + conflict) _conflicts+=("$conflict") ;; + provides) _provides+=("$provides") ;; + optdepend) _optdepends+=("$optdepend") ;; esac done< <(bsdtar -xOqf "$pkgfile" .PKGINFO) @@ -295,7 +297,7 @@ db_write_entry() { [[ -n $pkgbase ]] && echo -e "%BASE%\n$pkgbase\n" >>desc echo -e "%VERSION%\n$pkgver\n" >>desc [[ -n $pkgdesc ]] && echo -e "%DESC%\n$pkgdesc\n" >>desc - write_list_entry "GROUPS" "$_groups" "desc" + format_entry "GROUPS" "${_groups[@]}" >>"desc" [[ -n $csize ]] && echo -e "%CSIZE%\n$csize\n" >>desc [[ -n $size ]] && echo -e "%ISIZE%\n$size\n" >>desc @@ -307,20 +309,20 @@ db_write_entry() { [[ -n $pgpsig ]] && echo -e "%PGPSIG%\n$pgpsig\n" >>desc [[ -n $url ]] && echo -e "%URL%\n$url\n" >>desc - write_list_entry "LICENSE" "$_licenses" "desc" + format_entry "LICENSE" "${_licenses[@]}" >>"desc" [[ -n $arch ]] && echo -e "%ARCH%\n$arch\n" >>desc [[ -n $builddate ]] && echo -e "%BUILDDATE%\n$builddate\n" >>desc [[ -n $packager ]] && echo -e "%PACKAGER%\n$packager\n" >>desc - write_list_entry "REPLACES" "$_replaces" "desc" + format_entry "REPLACES" "${_replaces[@]}" >>"desc" # create depends entry msg2 "$(gettext "Creating '%s' db entry...")" 'depends' - # create the file even if it will remain empty - touch "depends" - write_list_entry "DEPENDS" "$_depends" "depends" - write_list_entry "CONFLICTS" "$_conflicts" "depends" - write_list_entry "PROVIDES" "$_provides" "depends" - write_list_entry "OPTDEPENDS" "$_optdepends" "depends" + { + format_entry "DEPENDS" "${_depends[@]}" + format_entry "CONFLICTS" "${_conflicts[@]}" + format_entry "PROVIDES" "${_provides[@]}" + format_entry "OPTDEPENDS" "${_optdepends[@]}" + } >'depends' popd >/dev/null popd >/dev/null -- 1.7.5.4
Allow one of 4 archive extensions: .tar{,.gz,.xz,.bz2} for each of the 2 valid repo extensions: .db and .files Signed-off-by: Dave Reisner <d@falconindy.com> --- scripts/repo-add.sh.in | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index ab97952..17639ed 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -573,10 +573,10 @@ if (( success )); then msg "$(gettext "Creating updated database file '%s'")" "$REPO_DB_FILE" case "$REPO_DB_FILE" in - *.tar.gz) TAR_OPT="z" ;; - *.tar.bz2) TAR_OPT="j" ;; - *.tar.xz) TAR_OPT="J" ;; - *.tar) TAR_OPT="" ;; + *.@(db|files).tar.gz) TAR_OPT="z" ;; + *.@(db|files).tar.bz2) TAR_OPT="j" ;; + *.@(db|files).tar.xz) TAR_OPT="J" ;; + *.@(db|files).tar) TAR_OPT="" ;; *) warning "$(gettext "'%s' does not have a valid archive extension.")" \ "$REPO_DB_FILE" ;; esac -- 1.7.5.4
On Wed, Jun 22, 2011 at 7:38 PM, Dave Reisner <d@falconindy.com> wrote:
Allow one of 4 archive extensions: .tar{,.gz,.xz,.bz2} for each of the 2 valid repo extensions: .db and .files
Good here, but- we should validate up front. What about moving this bit into a function that takes one argument (the db filename), and then calling that both when we first read the DB name, and then later when we need a tar flag? The function would return a tar compression flag, or "" for no compression, along with some designated invalid value that the initial call would check for and thus reject the database name. That way we don't need extension lists and expansion crazyness in more than one place.
b899099 made path checking a bit more strict than I had intended, and would actually forbid creation of a repo in $PWD if only the filename was specified. readlink would be the fun and easy solution here, but it's avoided due to portability issues, making the validation process a bit more verbose. --- scripts/repo-add.sh.in | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 17639ed..029e17d 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -374,9 +374,18 @@ db_remove_entry() { } # end db_remove_entry check_repo_db() { + local repodir + # ensure the path to the DB exists - if [[ ! -d "${LOCKFILE%/*}" ]]; then - error "$(gettext "%s does not exist or is not a directory.")" "${LOCKFILE%/*}" + if [[ "$LOCKFILE" == /* ]]; then + repodir=${LOCKFILE%/*}/ + else + repodir=$PWD/$LOCKFILE + repodir=${repodir%/*}/ + fi + + if [[ ! -d "$repodir" ]]; then + error "$(gettext "%s does not exist or is not a directory.")" "$repodir" exit 1 fi -- 1.7.5.4
Signed-off-by: Dave Reisner <d@falconindy.com> --- scripts/repo-add.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 029e17d..e75055f 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -593,7 +593,7 @@ if (( success )); then filename=${REPO_DB_FILE##*/} pushd "$tmpdir" >/dev/null - if [[ -n $(ls) ]]; then + if ( shopt -s nullglob; files=(*); (( ${#files[*]} )) ); then bsdtar -c${TAR_OPT}f "$filename" * else # we have no packages remaining? zip up some emptyness -- 1.7.5.4
On Wed, Jun 22, 2011 at 7:38 PM, Dave Reisner <d@falconindy.com> wrote: I agree with the commit message, but damn is this ugly. Can't we drop the whole nullglob bit and just do something like: contents=* if [[ $contents = "*" ]]; then ?
On Thu, Jun 23, 2011 at 11:06:45PM -0500, Dan McGee wrote:
The * won't expand when assigned to a variable, only to an array. So, in theory: contents=(*) if [[ $contents = "*" ]]; then But the nullglob is safer. I'm pretty sure almost _anything_ that's considered best practice in Bash is going to be ugly by your standards. d
On Fri, Jun 24, 2011 at 6:34 AM, Dave Reisner <d@falconindy.com> wrote:
martti@deepthought:~/test$ touch \* martti@deepthought:~/test$ contents=(*) martti@deepthought:~/test$ echo "${contents[*]}" * martti@deepthought:~/test$ echo ${#contents} 1 Don't approve. Dave's version fails if there's a valid file called '*', as little as it's probable. Better do bash ugly and safe. :)
On Wed, Jun 22, 2011 at 7:38 PM, Dave Reisner <d@falconindy.com> wrote:
Let's take a step back and just kill the entire conditional: bsdtar -c${TAR_OPT}f "$filename" -s /^.\// . "Add the current directory, but we don't want the ./ bits on the added paths."
If the repo file already exists, we allow specifying the symlink instead of the tarball itself. Signed-off-by: Dave Reisner <d@falconindy.com> --- I'm fairly sure this is a safe use of readlink, since it doesn't involve any of the flags for canonicalizing paths. I was able to test it on OSX with great success. scripts/repo-add.sh.in | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 8865188..3581765 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -582,7 +582,10 @@ done if (( success )); then msg "$(gettext "Creating updated database file '%s'")" "$REPO_DB_FILE" - case "$REPO_DB_FILE" in + # allows for syntax such as: repo-add repo.db /path/to/pkg + [[ -L "$REPO_DB_FILE" ]] && resolved=$(readlink "$REPO_DB_FILE") + + case "${resolved:-$REPO_DB_FILE}" in *.@(db|files).tar.gz) TAR_OPT="z" ;; *.@(db|files).tar.bz2) TAR_OPT="j" ;; *.@(db|files).tar.xz) TAR_OPT="J" ;; -- 1.7.5.4
On Wed, Jun 22, 2011 at 7:38 PM, Dave Reisner <d@falconindy.com> wrote:
Part of me says this still doesn't feel quite right. What if I just *move* my repo to repo.db? Is there a way to get bsdtar to use the same compression as another file, or some trick with file, like what we do in makepkg (search for 'application/', that will allow us to do this?
On Wed, Jun 22, 2011 at 08:38:44PM -0400, Dave Reisner wrote:
And yes, 3 of the emails are fubar because for some reason gmail decided to reject my credentials and claim i wasn't logged in. d
participants (3)
-
Dan McGee
-
Dave Reisner
-
Martti Kühne