Simple fix for FS#13414
Signed-off-by: Xavier Chantry shiningxc@gmail.com --- scripts/repo-add.sh.in | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index c6d25aa..b12188c 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -172,12 +172,16 @@ db_write_entry() return 1 fi
- # remove an existing entry if it exists, ignore failures - db_remove_entry "$pkgname" - startdir=$(pwd) pushd "$gstmpdir" 2>&1 >/dev/null
+ if [ -d "$pkgname-$pkgver" ]; then + warning "$(gettext "An entry for '%s' already existed")" "$pkgname-$pkgver" + fi + + # remove an existing entry if it exists, ignore failures + db_remove_entry "$pkgname" + # create package directory mkdir "$pkgname-$pkgver" cd "$pkgname-$pkgver"
Before this commit, the repo creation could fail after all packages have been added to the database. Now this will be detected before adding anything.
Signed-off-by: Xavier Chantry shiningxc@gmail.com --- scripts/repo-add.sh.in | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index b12188c..a967506 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -326,9 +326,21 @@ for arg in "$@"; do fi msg "$(gettext "Extracting database to a temporary location...")" bsdtar -xf "$REPO_DB_FILE" -C "$gstmpdir" - elif [ "$cmd" == "repo-remove" ]; then - error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE" - exit 1 + else + case "$cmd" in + repo-remove) + error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE" + exit 1 + ;; + repo-add) + # check if the file can be created (write permission, directory existence, etc) + if ! touch "$REPO_DB_FILE"; then + error "$(gettext "Repository file '%s' could not be created.")" "$REPO_DB_FILE" + exit 1 + fi + rm -f "$REPO_DB_FILE" + ;; + esac fi else if [ "$cmd" == "repo-add" ]; then
Refactor the main loop, which was difficult to read.
Use case instead of if when appropriate.
Signed-off-by: Xavier Chantry shiningxc@gmail.com --- scripts/repo-add.sh.in | 130 +++++++++++++++++++++++++---------------------- 1 files changed, 69 insertions(+), 61 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index a967506..0006006 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -270,6 +270,58 @@ db_remove_entry() { popd 2>&1 >/dev/null } # end db_remove_entry
+check_repo_db() +{ + if [ -f "$REPO_DB_FILE" ]; then + if ! (bsdtar -tf "$REPO_DB_FILE" | grep -q "/desc"); then + error "$(gettext "Repository file '%s' is not a proper pacman database.")" "$REPO_DB_FILE" + exit 1 + fi + msg "$(gettext "Extracting database to a temporary location...")" + bsdtar -xf "$REPO_DB_FILE" -C "$gstmpdir" + else + case "$cmd" in + repo-remove) + error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE" + exit 1 + ;; + repo-add) + # check if the file can be created (write permission, directory existence, etc) + if ! touch "$REPO_DB_FILE"; then + error "$(gettext "Repository file '%s' could not be created.")" "$REPO_DB_FILE" + exit 1 + fi + rm -f "$REPO_DB_FILE" + ;; + esac + fi +} + +add() +{ + pkgfile=$1 + if [ ! -f "$1" ]; then + error "$(gettext "Package '%s' not found.")" "$pkgfile" + return 1 + fi + + if ! bsdtar -tf "$pkgfile" .PKGINFO 2>&1 >/dev/null; then + error "$(gettext "'%s' is not a package file, skipping")" "$pkgfile" + return 1 + fi + + msg "$(gettext "Adding package '%s'")" "$pkgfile" + + db_write_entry "$pkgfile" +} + +remove() +{ + msg "$(gettext "Searching for package '%s'...")" "$arg" + + db_remove_entry "$arg" +} + # PROGRAM START
# determine whether we have gettext; make it a no-op if we do not @@ -279,17 +331,10 @@ if [ ! $(type -t gettext) ]; then } fi
-# check for help flags -if [ "$1" = "-h" -o "$1" = "--help" ]; then - usage - exit 0 -fi - -# check for version flags -if [ "$1" = "-V" -o "$1" = "--version" ]; then - version - exit 0 -fi +case "$1" in + -h|--help) usage; exit 0;; + -V|--version) version; exit 0;; +esac
# check for correct number of args if [ $# -lt 2 ]; then @@ -312,64 +357,27 @@ fi success=0 # parse arguments for arg in "$@"; do - if [ "$arg" == "--force" -o "$arg" == "-f" ]; then + case "$arg" in + -q|--quiet) QUIET=1;; + -f|--force) warning "$(gettext "the -f and --force options are no longer recognized")" msg2 "$(gettext "use options=(force) in the PKGBUILD instead")" - elif [ "$arg" == "--quiet" -o "$arg" == "-q" ]; then - QUIET=1 - elif [ -z "$REPO_DB_FILE" ]; then - REPO_DB_FILE="$arg" - if [ -f "$REPO_DB_FILE" ]; then - if ! (bsdtar -tf "$REPO_DB_FILE" | grep -q "/desc"); then - error "$(gettext "Repository file '%s' is not a proper pacman database.")" "$REPO_DB_FILE" - exit 1 - fi - msg "$(gettext "Extracting database to a temporary location...")" - bsdtar -xf "$REPO_DB_FILE" -C "$gstmpdir" + ;; + *) + if [ -z "$REPO_DB_FILE" ]; then + REPO_DB_FILE="$arg" + check_repo_db else case "$cmd" in - repo-remove) - error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE" - exit 1 - ;; - repo-add) - # check if the file can be created (write permission, directory existence, etc) - if ! touch "$REPO_DB_FILE"; then - error "$(gettext "Repository file '%s' could not be created.")" "$REPO_DB_FILE" - exit 1 - fi - rm -f "$REPO_DB_FILE" - ;; + repo-add) add $arg && success=1 ;; + repo-remove) remove $arg && success=1 ;; esac fi - else - if [ "$cmd" == "repo-add" ]; then - if [ -f "$arg" ]; then - if ! bsdtar -tf "$arg" .PKGINFO 2>&1 >/dev/null; then - error "$(gettext "'%s' is not a package file, skipping")" "$arg" - else - msg "$(gettext "Adding package '%s'")" "$arg" - - if db_write_entry "$arg"; then - success=1 - fi - fi - else - error "$(gettext "Package '%s' not found.")" "$arg" - fi - elif [ "$cmd" == "repo-remove" ]; then - msg "$(gettext "Searching for package '%s'...")" "$arg" - - if db_remove_entry "$arg"; then - success=1 - else - error "$(gettext "Package matching '%s' not found.")" "$arg" - fi - fi - fi + ;; + esac done
-# if all operations were a success, re-zip database +# if at least one operation was a success, re-zip database if [ $success -eq 1 ]; then msg "$(gettext "Creating updated database file '%s'")" "$REPO_DB_FILE"
* report when a package entry to be removed is not found
* backup and restore eventual "deltas" files
* slight optimization when looking for an entry : only look at the entries starting with $pkgname
Signed-off-by: Xavier Chantry shiningxc@gmail.com --- scripts/repo-add.sh.in | 50 ++++++++++++++++++++++++++++++++++++----------- 1 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 0006006..c21a08f 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -93,6 +93,20 @@ write_list_entry() { fi }
+find_pkgentry() +{ + local pkgname=$1 + local pkgentry + for pkgentry in $gstmpdir/$pkgname*; do + name=${pkgentry##*/} + if [ "${name%-*-*}" = "$pkgname" ]; then + echo $pkgentry + return 0 + fi + done + return 1 +} + # write a delta entry to the pacman database # arg1 - path to delta db_write_delta() @@ -186,6 +200,9 @@ db_write_entry() mkdir "$pkgname-$pkgver" cd "$pkgname-$pkgver"
+ # restore an eventual deltas file + [ -f "../$pkgname.deltas" ] && mv "../$pkgname.deltas" deltas + # create desc entry msg2 "$(gettext "Creating 'desc' db entry...")" echo -e "%FILENAME%\n$(basename "$1")\n" >>desc @@ -256,18 +273,20 @@ db_write_entry() # remove existing entries from the DB # arg1 - package name db_remove_entry() { - pushd "$gstmpdir" 2>&1 >/dev/null - - # remove any other package in the DB with same name - local existing - for existing in *; do - if [ "${existing%-*-*}" = "$1" ]; then - msg2 "$(gettext "Removing existing package '%s'...")" "$existing" - rm -rf "$existing" + local pkgname=$1 + local notfound=1 + local pkgentry=$(find_pkgentry $pkgname) + while [ -n "$pkgentry" ]; do + notfound=0 + if [ -f "$pkgentry/deltas" ]; then + mv "$pkgentry/deltas" "$gstmpdir/$pkgname.deltas" fi + msg2 "$(gettext "Removing existing package '%s'...")" \ + "$(basename $pkgentry)" + rm -rf $pkgentry + pkgentry=$(find_pkgentry $pkgname) done - - popd 2>&1 >/dev/null + return $notfound } # end db_remove_entry
check_repo_db() @@ -317,9 +336,16 @@ add()
remove() { - msg "$(gettext "Searching for package '%s'...")" "$arg" + pkgname=$1 + msg "$(gettext "Searching for package '%s'...")" "$pkgname"
- db_remove_entry "$arg" + if db_remove_entry "$pkgname"; then + rm -f "$gstmpdir/$pkgname.deltas" + return 0 + else + error "$(gettext "Package matching '%s' not found.")" "$pkgname" + return 1 + fi }
# PROGRAM START
The current implementation has several problems :
Wrong database format
All the info is taken from the filename, which is a bit ugly
It looks for .delta files in the current directory when adding a package, which is not very flexible
Signed-off-by: Xavier Chantry shiningxc@gmail.com --- scripts/repo-add.sh.in | 59 ------------------------------------------------ 1 files changed, 0 insertions(+), 59 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index c21a08f..9cf66e5 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -107,37 +107,6 @@ find_pkgentry() return 1 }
-# write a delta entry to the pacman database -# arg1 - path to delta -db_write_delta() -{ - # blank out all variables - local deltafile="$1" - local filename=$(basename "$deltafile") - local deltavars pkgname fromver tover arch csize md5sum - - # format of the delta filename: - # (package)-(fromver)_to_(tover)-(arch).delta - deltavars=( $(echo "$filename" | sed -e 's/(.*)-(.*-.*)_to_(.*-.*)-(.*).delta/\1 \2 \3 \4/') ) - pkgname=${deltavars[0]} - fromver=${deltavars[1]} - tover=${deltavars[2]} - arch=${deltavars[3]} - - # get md5sum and size of delta - md5sum="$(openssl dgst -md5 "$deltafile" | awk '{print $NF}')" - csize=$(@SIZECMD@ "$deltafile") - - # ensure variables were found - if [ -z "$pkgname" -o -z "$fromver" -o -z "$tover" -o -z "$arch" ]; then - return 1 - fi - - # add the entry for this delta file - echo -e "$fromver $tover $csize $filename $md5sum" >>deltas -} # end db_write_delta - - # write an entry to the pacman database # arg1 - path to package db_write_entry() @@ -232,40 +201,12 @@ db_write_entry() write_list_entry "PROVIDES" "$_provides" "depends" write_list_entry "OPTDEPENDS" "$_optdepends" "depends"
- # create deltas entry if there are delta files - # Xav : why should deltas be in $startdir? - for delta in $startdir/$pkgname-*-*_to_*-*-$arch.delta; do - # This for loop also pulls in all files that start with the current package - # name and are followed by a -whatever. For instance, running this loop for - # gcc would also grab gcc-libs. To guard against this, compare the package - # name of the delta to the current package name. - local filename=$(basename "$delta") - local dpkgname="$(echo "$filename" | sed -e 's/(.*)-.*-.*_to_.*-.*-.*.delta/\1/')" - if [ "$pkgname" = "$dpkgname" -a -f "$delta" ]; then - # create deltas file if it does not already exist - if [ ! -f "deltas" ]; then - msg2 "$(gettext "Creating 'deltas' db entry...")" - echo -e "%DELTAS%" >>deltas - fi - - # write this delta entry - if db_write_delta "$delta"; then - msg2 "$(gettext "Added delta '%s'")" "$(basename "$delta")" - else - warning "$(gettext "Could not add delta '%s'")" "$(basename "$delta")" - fi - fi - done - # add the final newline - [ -f "deltas" ] && echo -e "" >>deltas - popd 2>&1 >/dev/null
# preserve the modification time # Xav : what for? pkgdir="$gstmpdir/$pkgname-$pkgver" touch -r "$pkgfile" "$pkgdir/desc" "$pkgdir/depends" - [ -f "$pkgdir/deltas" ] && touch -r "$pkgfile" "$pkgdir/deltas"
return 0 } # end db_write_entry
Use the correct database format
Use xdelta3 to get the source and destination files from the delta itself
Allow delta files to be added with repo-add just like package files. delta files can also be removed with repo-remove. This is simply done by looking for a .delta extension in the arguments, and calling the appropriate db_write_delta or db_remove_delta functions.
Example usage: repo-add repo/test.db.tar.gz repo/libx11-1.1.99.2-2-x86_64.pkg.tar.gz repo-add repo/test.db.tar.gz repo/libx11-1.1.5-2_to_1.1.99.2-2-x86_64.delta repo-remove repo/test.db.tar.gz libx11-1.1.5-2_to_1.1.99.2-2-x86_64.delta
Signed-off-by: Xavier Chantry shiningxc@gmail.com --- scripts/repo-add.sh.in | 102 +++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 97 insertions(+), 5 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 9cf66e5..504adfd 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -57,8 +57,8 @@ error() { # print usage instructions usage() { printf "repo-add, repo-remove (pacman) %s\n\n" "$myver" - printf "$(gettext "Usage: repo-add [-q] <path-to-db> <package> ...\n")" - printf "$(gettext "Usage: repo-remove [-q] <path-to-db> <packagename> ...\n\n")" + printf "$(gettext "Usage: repo-add [-q] <path-to-db> <package|delta> ...\n")" + printf "$(gettext "Usage: repo-remove [-q] <path-to-db> <packagename|delta> ...\n\n")" printf "$(gettext "\ repo-add will update a package database by reading a package file.\n\ Multiple packages to add can be specified on the command line.\n\n")" @@ -107,6 +107,73 @@ find_pkgentry() return 1 }
+# Get the package name from the delta filename +get_delta_pkgname() { + local tmp + + tmp=${1##*/} + echo ${tmp%-*-*_to*} +} + +# write a delta entry +# arg1 - path to delta file +db_write_delta() +{ + deltafile="$1" + pkgname="$(get_delta_pkgname $deltafile)" + + pkgentry=$(find_pkgentry $pkgname) + if [ -z "$pkgentry" ]; then + return 1 + fi + deltas="$pkgentry/deltas" + # create deltas file if it does not already exist + if [ ! -f "$deltas" ]; then + msg2 "$(gettext "Creating 'deltas' db entry...")" + echo -e "%DELTAS%" >>$deltas + fi + # get md5sum and compressed size of package + md5sum="$(openssl dgst -md5 "$deltafile" | awk '{print $NF}')" + csize=$(@SIZECMD@ "$deltafile") + + oldfile=$(xdelta3 printhdr $deltafile | grep "XDELTA filename (source)" | sed 's/.*: *//') + newfile=$(xdelta3 printhdr $deltafile | grep "XDELTA filename (output)" | sed 's/.*: *//') + + if grep -q "$oldfile.*$newfile" $deltas; then + warning "$(gettext "An entry for '%s' already existed")" "$deltafile" + sed -i.backup "/$oldfile.*$newfile/d" $deltas && rm -f $deltas.backup + msg2 "$(gettext "Removing existing entry '%s'...")" "$deltafile" + fi + echo ${deltafile##*/} $md5sum $csize $oldfile $newfile >> $deltas + + return 0 +} # end db_write_delta + +# remove a delta entry +# arg1 - path to delta file +db_remove_delta() +{ + deltafile="$1" + filename=${deltafile##*/} + pkgname="$(get_delta_pkgname $deltafile)" + + pkgentry=$(find_pkgentry $pkgname) + if [ -z "$pkgentry" ]; then + return 1 + fi + deltas="$pkgentry/deltas" + if [ ! -f "$deltas" ]; then + return 1 + fi + if grep -q "$filename" $deltas; then + sed -i.backup "/$filename/d" $deltas && rm -f $deltas.backup + msg2 "$(gettext "Removing existing entry '%s'...")" "$filename" + return 0 + fi + + return 1 +} # end db_remove_delta + # write an entry to the pacman database # arg1 - path to package db_write_entry() @@ -222,7 +289,7 @@ db_remove_entry() { if [ -f "$pkgentry/deltas" ]; then mv "$pkgentry/deltas" "$gstmpdir/$pkgname.deltas" fi - msg2 "$(gettext "Removing existing package '%s'...")" \ + msg2 "$(gettext "Removing existing entry '%s'...")" \ "$(basename $pkgentry)" rm -rf $pkgentry pkgentry=$(find_pkgentry $pkgname) @@ -259,12 +326,26 @@ check_repo_db()
add() { - pkgfile=$1 if [ ! -f "$1" ]; then - error "$(gettext "Package '%s' not found.")" "$pkgfile" + error "$(gettext "File '%s' not found.")" "$1" return 1 fi
+ if [ "${1##*.}" == "delta" ]; then + deltafile=$1 + msg "$(gettext "Adding delta '%s'")" "$deltafile" + if [ ! "$(type -p xdelta3)" ]; then + error "$(gettext "Cannot find the xdelta3 binary! Is xdelta3 installed?")" + exit 1 + fi + if db_write_delta "$deltafile"; then + return 0 + else + return 1 + fi + fi + + pkgfile=$1 if ! bsdtar -tf "$pkgfile" .PKGINFO 2>&1 >/dev/null; then error "$(gettext "'%s' is not a package file, skipping")" "$pkgfile" return 1 @@ -277,6 +358,17 @@ add()
remove() { + if [ "${1##*.}" == "delta" ]; then + deltafile=$1 + msg "$(gettext "Searching for delta '%s'...")" "$deltafile" + if db_remove_delta "$deltafile"; then + return 0 + else + error "$(gettext "Delta matching '%s' not found.")" "$deltafile" + return 1 + fi + fi + pkgname=$1 msg "$(gettext "Searching for package '%s'...")" "$pkgname"
On Thu, Feb 26, 2009 at 2:06 PM, Xavier Chantry shiningxc@gmail.com wrote:
Refactor the main loop, which was difficult to read.
Use case instead of if when appropriate.
Signed-off-by: Xavier Chantry shiningxc@gmail.com
The idea is good, but make sure comments from previous patches make it into this reorg (formatting and the rm -f thing).
-Dan
Xavier Chantry wrote:
Before this commit, the repo creation could fail after all packages have been added to the database. Now this will be detected before adding anything.
Signed-off-by: Xavier Chantry shiningxc@gmail.com
Patch looks good. Minor stylistic comments below
scripts/repo-add.sh.in | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index b12188c..a967506 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -326,9 +326,21 @@ for arg in "$@"; do fi msg "$(gettext "Extracting database to a temporary location...")" bsdtar -xf "$REPO_DB_FILE" -C "$gstmpdir"
elif [ "$cmd" == "repo-remove" ]; thenerror "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE"exit 1
elsecase "$cmd" inrepo-remove)
indent the lines within this block some more:
error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE"exit 1;;repo-add)
and these
# check if the file can be created (write permission, directory existence, etc)if ! touch "$REPO_DB_FILE"; thenerror "$(gettext "Repository file '%s' could not be created.")" "$REPO_DB_FILE"exit 1firm -f "$REPO_DB_FILE";;esac
Allan
On Fri, Feb 27, 2009 at 1:56 AM, Allan McRae allan@archlinux.org wrote:
Xavier Chantry wrote:
Before this commit, the repo creation could fail after all packages have been added to the database. Now this will be detected before adding anything.
Signed-off-by: Xavier Chantry shiningxc@gmail.com
Patch looks good. Minor stylistic comments below
scripts/repo-add.sh.in | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index b12188c..a967506 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -326,9 +326,21 @@ for arg in "$@"; do fi msg "$(gettext "Extracting database to a temporary location...")" bsdtar -xf "$REPO_DB_FILE" -C "$gstmpdir"
- elif [ "$cmd" == "repo-remove" ]; then
- error "$(gettext "Repository file '%s' was not
found.")" "$REPO_DB_FILE"
- exit 1
- else
- case "$cmd" in
- repo-remove)
indent the lines within this block some more:
- error "$(gettext "Repository file '%s' was
not found.")" "$REPO_DB_FILE"
- exit 1
- ;;
- repo-add)
and these
- # check if the file can be created (write
permission, directory existence, etc)
- if ! touch "$REPO_DB_FILE"; then
- error "$(gettext "Repository file
'%s' could not be created.")" "$REPO_DB_FILE"
- exit 1
- fi
- rm -f "$REPO_DB_FILE"
- ;;
- esac
fi else if [ "$cmd" == "repo-add" ]; then
Yeah, I don't know why vim indent the blocks inside case like that... It is quite annoying. Is there a way to write case blocks more vim friendly, or some options / tricks I could use?
On Thu, Feb 26, 2009 at 2:06 PM, Xavier Chantry shiningxc@gmail.com wrote:
Before this commit, the repo creation could fail after all packages have been added to the database. Now this will be detected before adding anything.
Signed-off-by: Xavier Chantry shiningxc@gmail.com
scripts/repo-add.sh.in | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index b12188c..a967506 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -326,9 +326,21 @@ for arg in "$@"; do fi msg "$(gettext "Extracting database to a temporary location...")" bsdtar -xf "$REPO_DB_FILE" -C "$gstmpdir"
- elif [ "$cmd" == "repo-remove" ]; then
- error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE"
- exit 1
- else
- case "$cmd" in
- repo-remove)
- error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE"
- exit 1
- ;;
- repo-add)
- # check if the file can be created (write permission, directory existence, etc)
- if ! touch "$REPO_DB_FILE"; then
- error "$(gettext "Repository file '%s' could not be created.")" "$REPO_DB_FILE"
- exit 1
- fi
- rm -f "$REPO_DB_FILE"
This seems scary to me- is there any reason to blow it away like this? It also makes the repo-add process non-atomic- someone could access the database and fail while you are adding 10 packages, correct?
- ;;
- esac
fi else if [ "$cmd" == "repo-add" ]; then -- 1.6.1.3
Also agree with Allan's style comments, and I am not sure why vim indents like it does.
-Dan
Dan McGee wrote:
On Thu, Feb 26, 2009 at 2:06 PM, Xavier Chantry shiningxc@gmail.com wrote:
Before this commit, the repo creation could fail after all packages have been added to the database. Now this will be detected before adding anything.
Signed-off-by: Xavier Chantry shiningxc@gmail.com
scripts/repo-add.sh.in | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index b12188c..a967506 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -326,9 +326,21 @@ for arg in "$@"; do fi msg "$(gettext "Extracting database to a temporary location...")" bsdtar -xf "$REPO_DB_FILE" -C "$gstmpdir"
elif [ "$cmd" == "repo-remove" ]; thenerror "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE"exit 1
elsecase "$cmd" inrepo-remove)error "$(gettext "Repository file '%s' was not found.")" "$REPO_DB_FILE"exit 1;;repo-add)# check if the file can be created (write permission, directory existence, etc)if ! touch "$REPO_DB_FILE"; thenerror "$(gettext "Repository file '%s' could not be created.")" "$REPO_DB_FILE"exit 1firm -f "$REPO_DB_FILE"This seems scary to me- is there any reason to blow it away like this? It also makes the repo-add process non-atomic- someone could access the database and fail while you are adding 10 packages, correct?
I thought about this too but I was less scared when I noticed that this is only done when the db file is not found. So you are only removing the db file created in the touch statement.
On Sun, Mar 1, 2009 at 6:57 AM, Allan McRae allan@archlinux.org wrote:
I thought about this too but I was less scared when I noticed that this is only done when the db file is not found. So you are only removing the db file created in the touch statement.
Indeed. However when the db file is found, I didn't even check it was also writable in that case. And we have a race condition huge like a mountain here. There can be several minutes spent between this write/creation check is made, and the actual write of the new database. It would probably be a good idea to write a lock file for each database ($REPO.lck) so that there is never more than one repo-add instance running for one given db.
For these reasons, I will just drop that patch for now, for rewriting it later.
On Sat, Feb 28, 2009 at 10:37 PM, Dan McGee dpmcgee@gmail.com wrote:
Also agree with Allan's style comments, and I am not sure why vim indents like it does.
I guess vim considers case blocks just like any other blocks : " Add a 'shiftwidth' after if, while, else, case, until, for, function()
So it indents all the lines inside a case/esac at the same level. And it apparently does not consider case statements as blocks.
That indentation is made even worse by the fact that I didn't even put a newline between the different case statements, so it is even harder to see them. Anyway my updated patches have manual indentation now.
On Thu, Feb 26, 2009 at 2:06 PM, Xavier Chantry shiningxc@gmail.com wrote:
Simple fix for FS#13414
Signed-off-by: Xavier Chantry shiningxc@gmail.com
Signed-off-by: Dan McGee dan@archlinux.org
pacman-dev@lists.archlinux.org