[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] contrib/client-side/asvn: only check files, that are part of the subversion repository

From: Stefan Sperling <stsp_at_elego.de>
Date: Thu, 6 May 2010 12:52:58 +0200

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 @@
> }
> +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.


> + 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
Received on 2010-05-06 12:53:58 CEST

This is an archived mail posted to the Subversion Dev mailing list.