On Thu, May 06, 2010 at 12:02:36PM +0200, Jörg Steffens wrote:
> IMHO it is a cleaner approach to only work on files that are part of the
> svn checkout instead of calling "svn" for all files in the directory
> (and filter for errors). But of course, both approaches are doable.
>
> I replaced "stat -c" by "find -maxdepth 0".
> If this would work also with BSD, I would appreciate if my patch gets
> included.
>
> Index: contrib/client-side/asvn
> ===================================================================
> --- contrib/client-side/asvn (Revision 941610)
> +++ contrib/client-side/asvn (Arbeitskopie)
> @@ -61,6 +61,24 @@
> rm -f $TMPFILE $TMPFILE1 $TMPFILE2
> }
>
> +function stat_details()
> +{
> + local path=${1:-.}
> + # stat -c: can not be used, because it does not work on BSD
I think the above comment can just be removed. It talks about what
happened during patch review, rather than expaining why the code does
something the reader might not expect. A comment which talks about code
that isn't there isn't really useful.
Since the function doesn't use stat anymore, maybe rename it to
something like "get_file_info"?
> + find "$path" -maxdepth 0 -printf "mode=%m user=%u(%U) group=%g(%G)\n"
On BSD, this yields: find: -printf: unknown option
Which isn't your fault. It turns out that even as currently shipped,
asvn uses a few command line switches that are specific to GNU tools.
So asvn only works with GNU userland tools anyway right now.
For instance, without your patch I already get errors like:
$ asvn ci foo -m "adding foo"
this is the pre checkin process
find: -false: unknown option
find: -false: unknown option
Adding foo
Transmitting file data .
Committed revision 3.
Rewriting asvn to be portable to non-GNU system should be left for
a different patch. I'm not saying that you have to do that, just that
all patches should be self-contained on focus on solving a single problem.
Right now the focus is on preventing asvn to consider files which aren't
under version control, not on making asvn work on non-GNU systems.
> +}
> +
> +function svn_list()
> +{
> + [ $# -eq 0 ] && local path="`pwd`"
> + # grep -v "^?": exclude all files that do not belong to subversion
What about files that are ignored by svn?
They show up as 'I' if explicitly mentioned on the command line:
$ svn ps svn:ignore foo .
$ touch foo
$ svn status
$ svn status foo
I foo
$
> + # cut -c 42-: svn status lists different information. The filename
> starts at column 42
Language nitpick: "different" to what? I think you meant to say
something like "various information". Maybe just assume that people
already know what svn status does, and say:
# cut -c42 because filenames start at column 42 in svn status -v output
> + # improvement: use "svn status --xml" and parse the xml output
I don't think parsing XML in a shell script will be an improvement :)
The script would need to rely on some external tool to parse the XML.
XML wasn't made to be parsed by UNIX shell scripts.
> + svn status -v --ignore-externals $path "$@" | grep -v "^?" | cut -c 42-
Note that externals still show up as 'X' even with --ignore-externals.
Why do you always ignore externals?
svn commit does not recurse into externals by default, but svn update does.
So it seems as if record{dirinfo,permissions} should be run for all externals
after an update, and for commit, record{dirinfo,permissions} should be run
for externals which are explicitly mentioned on the command line (because
svn commit will recurse into them). Is this correct and can it be
done easily?
> +}
> +
> +
> +
> function basedirname()
> {
> refname="$1"
> @@ -320,10 +338,12 @@
> # Find all the directories and files
> cp /dev/null $TMPFILE
>
> - eval "find $PCWD $SKIPSVN -o \( \( -type d ! -name .svn \) -o -type f
> \) $PRINTDETAILS" | while read info
> + # uses svn_list instead of version based on find,
> + # because the find version produces warnings for all files
> + # that are not part of the repository (eg. backup files)
The above 3 lines of comment can also be removed.
Thanks,
Stefan
> + svn_list $PCWD | while read device
> do
> - device=`expr "$info" : "file='\(.*\)' mode"`
> - info=`expr "$info" : "file='.*' \(mode.*\)"`
> + info=`stat_details "$device"`
>
> if [ "$PCWD" = "$device" ]
> then
