You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jörg Steffens <jo...@dass-it.de> on 2010/05/05 17:03:44 UTC
[PATCH] contrib/client-side/asvn: only check files, that are part
of the subversion repository
[[[
originally, asvn uses find commands to determine the files where
properties are set. The new version uses svn status. Therefore only
files that are part of the repository are used. This reduces warning
messages.
* contrib/client-side/asvn
(recordpermissions): uses the new function svn_list instead of find
patch by joerg.steffens < at > dass-it.de
]]]
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Jörg Steffens <jo...@dass-it.de>.
Am 06.05.2010 16:06, schrieb Stefan Sperling:
> On Thu, May 06, 2010 at 03:16:38PM +0200, Jörg Steffens wrote:
>> [[[
>> reduces error messages by checking svn return code
>>
>> * contrib/client-side/asvn
>> (recordpermissions): skip files where "svn propget" returns an error.
>> It is assumed that these files do not belong to the svn repository
>>
>> patch by joerg.steffens < at > dass-it.de
>> ]]]
> 'if [ x ]; then' is not consistent with the rest of the file.
> Usually 'then' is on a line of its own, like this:
>
> if [ x ]
> then
>
>> + # $file is not handled by svn, skipping
>
> Let's zap the above comment,
>
>> + echo "skipping property $FILE_PROP for $dir/$file"
>
> and change this messsage to:
>
> $dir/$file has no $FILE_PROP property or is unversioned; skipping
okay. Here it is.
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Jörg Steffens <jo...@dass-it.de>.
Am 06.05.2010 16:06, schrieb Stefan Sperling:
> On Thu, May 06, 2010 at 03:16:38PM +0200, Jörg Steffens wrote:
>> [[[
>> reduces error messages by checking svn return code
>>
>> * contrib/client-side/asvn
>> (recordpermissions): skip files where "svn propget" returns an error.
>> It is assumed that these files do not belong to the svn repository
>>
>> patch by joerg.steffens < at > dass-it.de
>> ]]]
> 'if [ x ]; then' is not consistent with the rest of the file.
> Usually 'then' is on a line of its own, like this:
>
> if [ x ]
> then
>
>> + # $file is not handled by svn, skipping
>
> Let's zap the above comment,
>
>> + echo "skipping property $FILE_PROP for $dir/$file"
>
> and change this messsage to:
>
> $dir/$file has no $FILE_PROP property or is unversioned; skipping
okay. Here it is.
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Stefan Sperling <st...@elego.de>.
On Thu, May 06, 2010 at 03:16:38PM +0200, Jörg Steffens wrote:
> [[[
> reduces error messages by checking svn return code
>
> * contrib/client-side/asvn
> (recordpermissions): skip files where "svn propget" returns an error.
> It is assumed that these files do not belong to the svn repository
>
> patch by joerg.steffens < at > dass-it.de
> ]]]
> Index: contrib/client-side/asvn
> ===================================================================
> --- contrib/client-side/asvn (Revision 941610)
> +++ contrib/client-side/asvn (Arbeitskopie)
> @@ -335,7 +335,11 @@
> fi
>
> # see if the properties have changed.
> - if [ "`$SVN propget $FILE_PROP \"$dir/$file\"`" != "$info" ]
> + FILE_PROP_VALUE=`$SVN propget $FILE_PROP "$dir/$file" 2>/dev/null`
> + if [ $? -ne 0 ]; then
'if [ x ]; then' is not consistent with the rest of the file.
Usually 'then' is on a line of its own, like this:
if [ x ]
then
> + # $file is not handled by svn, skipping
Let's zap the above comment,
> + echo "skipping property $FILE_PROP for $dir/$file"
and change this messsage to:
$dir/$file has no $FILE_PROP property or is unversioned; skipping
Then I'd be happy to commit this.
Thanks,
Stefan
> + elif [ "$FILE_PROP_VALUE" != "$info" ]
> then
> if [ "$CHECKIN" = "true" ]
> then
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Stefan Sperling <st...@elego.de>.
On Thu, May 06, 2010 at 03:16:38PM +0200, Jörg Steffens wrote:
> [[[
> reduces error messages by checking svn return code
>
> * contrib/client-side/asvn
> (recordpermissions): skip files where "svn propget" returns an error.
> It is assumed that these files do not belong to the svn repository
>
> patch by joerg.steffens < at > dass-it.de
> ]]]
> Index: contrib/client-side/asvn
> ===================================================================
> --- contrib/client-side/asvn (Revision 941610)
> +++ contrib/client-side/asvn (Arbeitskopie)
> @@ -335,7 +335,11 @@
> fi
>
> # see if the properties have changed.
> - if [ "`$SVN propget $FILE_PROP \"$dir/$file\"`" != "$info" ]
> + FILE_PROP_VALUE=`$SVN propget $FILE_PROP "$dir/$file" 2>/dev/null`
> + if [ $? -ne 0 ]; then
'if [ x ]; then' is not consistent with the rest of the file.
Usually 'then' is on a line of its own, like this:
if [ x ]
then
> + # $file is not handled by svn, skipping
Let's zap the above comment,
> + echo "skipping property $FILE_PROP for $dir/$file"
and change this messsage to:
$dir/$file has no $FILE_PROP property or is unversioned; skipping
Then I'd be happy to commit this.
Thanks,
Stefan
> + elif [ "$FILE_PROP_VALUE" != "$info" ]
> then
> if [ "$CHECKIN" = "true" ]
> then
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Jörg Steffens <jo...@dass-it.de>.
Am 06.05.2010 12:52, schrieb Stefan Sperling:
[...]
thank you for your comments.
Unfortunately, I've found another problem in my patch:
# cut -c 42-: svn status lists different information. The filename
starts at column 42
is not even vaild for all Linux systems.
As you recommended in your first email, the new patch just checks for
errors and skip the remaining calls.
Your are right, when you point out, that there are still a lot of cases
where this small wrapper script fails. But at least, this patch reduces
the number of problems by one.
New patch desciption:
[[[
reduces error messages by checking svn return code
* contrib/client-side/asvn
(recordpermissions): skip files where "svn propget" returns an error.
It is assumed that these files do not belong to the svn repository
patch by joerg.steffens < at > dass-it.de
]]]
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Stefan Sperling <st...@elego.de>.
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
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Stefan Sperling <st...@elego.de>.
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
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Jörg Steffens <jo...@dass-it.de>.
Am 05.05.2010 21:15, schrieb Stefan Sperling:
> On Wed, May 05, 2010 at 07:03:44PM +0200, Jörg Steffens wrote:
>> +function stat_details()
>> +{
>> + # attention: stat uses other variables as find, eg. %u<->%U
>> + local path=${1:-.}
>> + stat -c "mode=%a user=%U(%u) group=%G(%g)" "$path"
>
> This isn't portable, e.g. on OpenBSD you get:
>
> stat: unknown option -- c
> usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file ...]
>
> Is there a more portable way to get at this information?
>
> In general it's quite hard to write shell scripts that run everywhere.
> Maybe just keep the original find-based code, but filter some errors?
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.
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Jörg Steffens <jo...@dass-it.de>.
Am 05.05.2010 21:15, schrieb Stefan Sperling:
> On Wed, May 05, 2010 at 07:03:44PM +0200, Jörg Steffens wrote:
>> +function stat_details()
>> +{
>> + # attention: stat uses other variables as find, eg. %u<->%U
>> + local path=${1:-.}
>> + stat -c "mode=%a user=%U(%u) group=%G(%g)" "$path"
>
> This isn't portable, e.g. on OpenBSD you get:
>
> stat: unknown option -- c
> usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file ...]
>
> Is there a more portable way to get at this information?
>
> In general it's quite hard to write shell scripts that run everywhere.
> Maybe just keep the original find-based code, but filter some errors?
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.
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Stefan Sperling <st...@elego.de>.
On Wed, May 05, 2010 at 07:03:44PM +0200, Jörg Steffens wrote:
> +function stat_details()
> +{
> + # attention: stat uses other variables as find, eg. %u<->%U
> + local path=${1:-.}
> + stat -c "mode=%a user=%U(%u) group=%G(%g)" "$path"
This isn't portable, e.g. on OpenBSD you get:
stat: unknown option -- c
usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file ...]
Is there a more portable way to get at this information?
In general it's quite hard to write shell scripts that run everywhere.
Maybe just keep the original find-based code, but filter some errors?
Stefan
Re: [PATCH] contrib/client-side/asvn: only check files, that are
part of the subversion repository
Posted by Stefan Sperling <st...@elego.de>.
On Wed, May 05, 2010 at 07:03:44PM +0200, Jörg Steffens wrote:
> +function stat_details()
> +{
> + # attention: stat uses other variables as find, eg. %u<->%U
> + local path=${1:-.}
> + stat -c "mode=%a user=%U(%u) group=%G(%g)" "$path"
This isn't portable, e.g. on OpenBSD you get:
stat: unknown option -- c
usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file ...]
Is there a more portable way to get at this information?
In general it's quite hard to write shell scripts that run everywhere.
Maybe just keep the original find-based code, but filter some errors?
Stefan