Re: [Quilt-dev] Another shell re-write of backup-files.

[Jean Delvare]

On Friday 18 March 2011 05:50:18 pm Kaz Kylheku wrote:
> On Fri, 18 Mar 2011 10:18:27 +0100, Jean Delvare <address@hidden>
> 
> wrote:
> > Hi Kaz,
> >
> > On Friday 18 March 2011 02:26:27 am Kaz Kylheku wrote:
> >> Hey everyone,
> >>
> >> Recently I became interested in a quilt that consists only of
> >> shell scripts.
> >
> > You're not the only one. I'm happy to see momentum grow in this
> > direction.
> 
> I did some further hacking on this last night, sliding the program
> under quilt and getting it to work, including adding new files,
> and pushes and pops with file creating/deleting patches.

That's not enough. Quilt comes with a non-regression test suite, which 
your script should pass. Try it: "make check". I did, your code failed 
(even the update you send half an hour ago.)

I also wrote a dedicated test case for backup-files, which I am 
attaching so that you can test your code against it. It assumes that 
backup-files is in quilt/scripts/, change the path if it isn't the case. 
You current code doesn't pass.

But again, please keep in mind that backup-files is not trivial (you'll 
understand when you see it fail the various test cases), and I have 
spent a lot of time on my own bash version of it. Once you'll be done 
with correctness, you'll have to deal with portability (my code works on 
many flavors of Linux + FreeBSD 8.1, does yours? I bet not) and 
performance (I see that your code creates 3 temporary files 
unconditionally, so I expect the performance of "quilt add" and "quilt 
remove" on single files to suffer.)

> This version does not change the backup format, so everything works.
> 
> I will fix the touch logic so that it doesn't diddle the backup
>  files. It's clear that we can't do a faithful restore with
>  timestamps intact if it's done the way  I have it.

Quick review, only pointing out the most obvious problems:

> #!/bin/bash
> 
> #
> # Shell script replacement for quilt's backup-files utility
> # Tested to some extent under quilt.
> # Mar 18, 2011
> # Kaz Kylheku <address@hidden>
> #
> 
> #
> # Main concepts:
> #
> # - Goal is to avoid invoking a process for each file name
> # - We use the CPIO utility for creating hard-linked backups; CPIO
> #   pass-through mode can take list of names and create hard links
>  (in #   pass-through mode).

You mentioned "pass-through mode" twice, this seems redundant.

> # - Use tree-to-tree recursive cp to restore a backup (taking
> everything
> #   in the backup, ignoring the file list).
> # - To touch files on restore, if requested, we can do a find + xargs
>  + #   touch over the backup instead, prior to restoring it. [WRONG]
>  # - Added files (i.e. backups of nonexistent files) are represented
>  as a
> #   specially named file containing an explicit list, and not as
> #   zero-length files. This eases the implementation, and lets us
>  back #   up/restore zero-length files!
> #
> 
> set -eu # bail on any errors, and unbound variable uses
> 
> opt_prefix=
> opt_suffix=
> opt_file=
> opt_backup=
> opt_restore=
> opt_remove_backup=
> opt_keep_backups=
> opt_silent=
> opt_touch=
> opt_nolink=
> 
> all_backup_files=
> 
> usage()
> {
> cat <<!
> Usage: $0 [-B prefix] [-f {file|-}] [-sktL] [-b|-r|-x] {file|-} ...
> 
>       Create hard linked backup copies of a list of files
>       read from a file (or standard input), or from the
>       argument list.
> 
>       The argument list "-" means "every regular file in
>       the backup directory specified by the -B prefix.
> 
>       -b      Create backup
>       -r      Restore the backup
>       -x      Remove backup files and empty parent directories
>       -k      When doing a restore, keep the backup files
>       -B      Path name prefix for backup files. Should have trailing slash.
>               Without a trailing slash, the behavior is different from the
>               C implementation.
>       -z      Unsupported, obsolete option

Why list it at all then?

>       -s      Silent operation; only print error messages
>       -f      Read the filenames to process from file (- = standard input)
>       -t      Touch original files after restore (update their mtimes)
> 
>       -L      Ensure that when finished, the source file has a link count of 1
> !
> }
> 
> if ! options=`getopt -o B:f:brxkstLh -- "$@"` ; then
>       usage
>       exit 1
> fi
> 
> eval set -- "$options"
> 
> while true
> do
>       case "$1" in
>       -B)
>               opt_prefix="$2"
>               shift 2 ;;
>       -f)
>               opt_file="$2"
>               shift 2 ;;
>       -b)
>               opt_backup=B
>               shift ;;
>       -r)
>               opt_restore=R
>               shift ;;
>       -x)
>               opt_remove_backup=D
>               shift ;;
>       -k)
>               opt_keep_backups=y
>               shift ;;
>       -s)
>               opt_silent=y
>               shift ;;
>       -t)
>               opt_touch=y
>               shift ;;
>       -L)
>               opt_nolink=y
>               shift ;;
>       -h)
>               usage
>               exit 0 ;;
>       --)
>               shift
>               break ;;
>       esac
> done
> 
> if [ $# -eq 0 -a -z "$opt_file" ] ; then
>       echo "Error: specify input file names as arguments or via -f option"
>       echo
>       usage
>       exit 1
> fi
> 
> if [ $# -ge 1 -a -n "$opt_file" ] ; then
>       echo "Error: conflict: both -f and file name argument given"
>       echo
>       usage
>       exit 1
> fi
> 
> if [ $# -gt 1 -a "$1" == "-" ] ; then
>       echo "Error: if - is specified, then no other arguments can be
>  added" echo
>       usage
>       exit 1
> fi
> 
> 
> if [ -z "$opt_prefix" ] ; then
>       echo "Error: specify backup/restore directory with -B"
>       echo
>       usage
>       exit 1
> fi
> 
> # temp file that holds raw list of names
> temp_list=$(mktemp "${TMP_DIR:-/tmp}/backup-files-tl-XXXXXX")
> 
> # temp file that holds names of files that exist
> file_list=$(mktemp "${TMP_DIR:-/tmp}/backup-files-fl-XXXXXX")
> 
> # temp file that holds names of nonexistent files
> noex_list=$(mktemp "${TMP_DIR:-/tmp}/backup-files-ne-XXXXXX")
> 
> cleanup()
> {
>       rm -f $temp_list $file_list $noex_list
> }
> 
> trap cleanup exit
> 
> #
> # capture the file list into the $temp_list file
> #
> 
> if [ -n "$opt_file" -a "$opt_file" == "-" ] ; then
>       cat > $temp_list
> elif [ -n "$opt_file" ] ; then
>       cat "$opt_file" > $temp_list
> elif [ "$1" == "-" ] ; then
>       all_backup_files=y
> else
>       # IFS trick. The string literal here contains a newline
>       ( IFS="
> "
>         echo "$*" > $temp_list )
> fi
> 
> #
> # separate name list into existing and nonexisting
> #
> 
> > $file_list
> > $noex_list
> 
> while read name ; do
>       if [ -e "$name" ] ; then
>               echo "$name" >> $file_list
>       else
>               echo "$name" >> $noex_list
>       fi
> done < $temp_list
> 
> case $opt_backup$opt_restore$opt_remove_backup in
> B )
>       if [ $all_backup_files ] ; then
>               echo "Error: \"-\" argument makes no sense for backup action"
>               echo
>               usage
>               exit 1
>       fi
> 
>       if [ $opt_nolink ] ; then
>               cpio --quiet -pd "$opt_prefix" < $file_list
>       else
>               cpio --quiet -pdl "$opt_prefix" < $file_list
>       fi

You have implemented the "no link" backup improperly. "No link" means 
that the _source_ ends unlinked. The backup is still linked to all other 
files the source was originally linked to. Your code does it the other 
way around.

> 
>       umask 0111 # so touched files are 0666

Why would you want touched files to be world-writable?

> 
>       # escaping issues here: we are assuming opt_prefix
>       # doesn't contain # or things interpreted by sed
>   sed -e "s#.*#$opt_prefix&#" < $noex_list | xargs touch
>       ;;
> R )
>       if [ -z $all_backup_files ] ; then
>               echo "Error: restoring individual files is not supported"
>               exit 1
>       fi

This is mandatory for "quilt remove", "quilt diff" and "quilt revert".

> 
>       if [ $opt_nolink ] ; then
>               cp -a "$opt_prefix"/. .

FWIW, this is never used by quilt.

>       else
>               if [ $opt_touch ] ; then
>                       find "$opt_prefix" -type f | xargs touch
>               fi
> 
>               cp -rlf "$opt_prefix"/. .

Not all implementations of cp support option -l, nor --
preserve=mode,ownership which you used in the update.

>       fi
> 
>       find "$opt_prefix" -type f -size 0c \
>         | cut -c "$((1+${#opt_prefix}))-" \
>         | xargs rm -f
> 
>       if [ -z "$opt_keep_backups" ] ; then
>               rm -rf "$opt_prefix"
>       fi
>       ;;
> D )
>       rm -rf "$opt_prefix"
>       ;;

FWIW, quilt doesn't use this.

> * )
>       # either no operation was specified, or multiple operations
>       # were been specified, like -b and -r.
>       usage
>       exit 1
>       ;;
> esac
> 

But then again... it seems to be a waste of time that you work on a 
problem which is already solved.

-- 
Jean Delvare
Suse L3

backup-files.test
Description: Text document