[PATCH 1/3] lib/bup/drecurse.py: recognize files from excluded paths, not only directories.

[Jakob Matthes]

Signed-off-by: Jakob Matthes <jakob....@googlemail.com>
---
lib/bup/drecurse.py | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/bup/drecurse.py b/lib/bup/drecurse.py
index 866045f..536a32a 100644
--- a/lib/bup/drecurse.py
+++ b/lib/bup/drecurse.py
@@ -73,7 +73,13 @@ def _recursive_dirlist(prepend, xdev, bup_dir=None, excluded_paths=None):
excluded_paths=excluded_paths):
yield i
os.chdir('..')
- yield (prepend + name, pst)
+ else:
+ if excluded_paths:
+ if os.path.normpath(prepend+name) in excluded_paths:
+ debug1('Skipping %r: excluded.\n' % (prepend+name))
+ continue
+
+ yield (prepend+name, pst)


def recursive_dirlist(paths, xdev, bup_dir=None, excluded_paths=None):
--
1.7.9.2

[Jakob Matthes]

A pretty basic function to reduce some nesting and duplication
introduced by the previous patch. This patch can be omitted without
affecting the other two.

Signed-off-by: Jakob Matthes <jakob....@googlemail.com>
---

lib/bup/drecurse.py | 14 ++++++--------
lib/bup/helpers.py | 9 +++++++++
2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/lib/bup/drecurse.py b/lib/bup/drecurse.py
index 536a32a..66efd47 100644
--- a/lib/bup/drecurse.py
+++ b/lib/bup/drecurse.py
@@ -59,10 +59,9 @@ def _recursive_dirlist(prepend, xdev, bup_dir=None, excluded_paths=None):
if os.path.normpath(prepend+name) == bup_dir:
debug1('Skipping BUP_DIR.\n')
continue
- if excluded_paths:
- if os.path.normpath(prepend+name) in excluded_paths:
- debug1('Skipping %r: excluded.\n' % (prepend+name))
- continue
+ if path_in(os.path.normpath(prepend+name), excluded_paths):

+ debug1('Skipping %r: excluded.\n' % (prepend+name))
+ continue

try:
OsFile(name).fchdir()
except OSError, e:
@@ -74,10 +73,9 @@ def _recursive_dirlist(prepend, xdev, bup_dir=None, excluded_paths=None):

yield i
os.chdir('..')

else:
- if excluded_paths:
- if os.path.normpath(prepend+name) in excluded_paths:
- debug1('Skipping %r: excluded.\n' % (prepend+name))
- continue
+ if path_in(os.path.normpath(prepend+name), excluded_paths):

+ debug1('Skipping %r: excluded.\n' % (prepend+name))
+ continue

yield (prepend+name, pst)

diff --git a/lib/bup/helpers.py b/lib/bup/helpers.py
index d9d177c..770875e 100644
--- a/lib/bup/helpers.py
+++ b/lib/bup/helpers.py
@@ -197,6 +197,15 @@ def realpath(p):
#log('realpathing:%r,%r\n' % (p, out))
return out

+def path_in(p, paths):
+ """Check if a given path is contained in a list of paths.
+
+ The list 'paths' can be None in which case it returns False."""
+ if paths == None:
+ return False
+ else:
+ return p in paths
+

def detect_fakeroot():
"Return True if we appear to be running under fakeroot."
--
1.7.9.2

[Jakob Matthes]

Catch the correct exception and hence display the warning. There is a
'class Error' definition in lib/bup/index.py but I am not clear about
its usefulness.

Wrap f.close() in a try...except statement to avoid UnboundLocalError.
Maybe the nesting can be reduced without losing Python 2.4 compatibility
or missing exception cases.

Signed-off-by: Jakob Matthes <jakob....@googlemail.com>
---

lib/bup/drecurse.py | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/bup/drecurse.py b/lib/bup/drecurse.py
index 66efd47..6036e84 100644
--- a/lib/bup/drecurse.py
+++ b/lib/bup/drecurse.py
@@ -135,10 +135,14 @@ def parse_excludes(flags):
f = open(realpath(parameter))
for exclude_path in f.readlines():
excluded_paths.append(realpath(exclude_path.strip()))
- except Error, e:
+ except Exception, e:
log("warning: couldn't read %s\n" % parameter)
finally:
- f.close()
+ try:
+ f.close()
+ except Exception, e:
+ pass
+

return excluded_paths

--
1.7.9.2

[Gabriel Filion]

On 12-02-26 06:36 PM, Jakob Matthes wrote:
> f = open(realpath(parameter))
> for exclude_path in f.readlines():
> excluded_paths.append(realpath(exclude_path.strip()))
> - except Error, e:
> + except Exception, e:
> log("warning: couldn't read %s\n" % parameter)

This was just mentioned in a pull request on github:

https://github.com/apenwarr/bup/pull/12

Alex suggests the same fix as the above.

I traced the history of this line, and it was added in commit
a1d5ae13d57, and then moved around until it was in drecurse.py.

The line was initially in index-cmd.py, but the Error class has been in
a library for quite a while.

The problem I see with this change is if realpath() throws an unexpected
exception, it'll just be grabbed by the above and logged on the screen
as a warning (which you proabably won't see if you're not attached to a
tty..)

Zoran: Was there a reason to catch "Error" in particular?
From the looks of it, the try/catch is there for the call to open(), so
shouldn't we change that catch for something more specific like
IOError[1]? Or is there something that can happen inside realpath() ?

[1]: http://docs.python.org/library/functions.html#open

--
Gabriel Filion

[Gabriel Filion]

On 12-02-26 06:36 PM, Jakob Matthes wrote:

> log("warning: couldn't read %s\n" % parameter)
> finally:
> - f.close()
> + try:
> + f.close()
> + except Exception, e:
> + pass
> +

I haven't seen in the documentation[1] that f.close() can send an
exception. Why is this part necessary? Are there some undocumented cases
where it raises an exception?

[1]: http://docs.python.org/library/stdtypes.html#file.close

--
Gabriel Filion

[Gabriel Filion]

On 12-02-26 06:36 PM, Jakob Matthes wrote:

> os.chdir('..')
> - yield (prepend + name, pst)
> + else:
> + if excluded_paths:
> + if os.path.normpath(prepend+name) in excluded_paths:
> + debug1('Skipping %r: excluded.\n' % (prepend+name))
> + continue
> +
> + yield (prepend+name, pst)
>
>
> def recursive_dirlist(paths, xdev, bup_dir=None, excluded_paths=None):

Just by reading, this commit looks good to me. I'll try and test this
out soon.

Jakob, can you come up a test that verifies that a normal file is
excluded when specified in --exclude ?

--
Gabriel Filion

[Gabriel Filion]

I'm not really sure about this one ...

It looks syntactically valid, but it actually adds code and makes it a
bit tougher to read.. If the comparison is used in more places, we can
do this modification, but I don't think it's useful for just two occurences.

On 12-02-26 06:36 PM, Jakob Matthes wrote:

--
Gabriel Filion

[Alex Morega]

The Error class doesn't exist. If an exception is raised (e.g. the
file mentioned in --exclude-from doesn't exist), then a 2nd exception
is raised, `NameError: global name 'Error' is not defined`.

I agree that catching IOError is more appropriate.

[Jakob Matthes]

Signed-off-by: Jakob Matthes <jakob....@gmail.com>
---
lib/bup/drecurse.py | 6 ++++++
t/test.sh | 7 +++++--
2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/bup/drecurse.py b/lib/bup/drecurse.py
index 866045f..f72be25 100644
--- a/lib/bup/drecurse.py
+++ b/lib/bup/drecurse.py
@@ -73,6 +73,12 @@ def _recursive_dirlist(prepend, xdev, bup_dir=None, excluded_paths=None):
excluded_paths=excluded_paths):

yield i
os.chdir('..')

+ else:
+ if excluded_paths:
+ if os.path.normpath(prepend+name) in excluded_paths:
+ debug1('Skipping %r: excluded.\n' % (prepend+name))
+ continue
+
yield (prepend + name, pst)

diff --git a/t/test.sh b/t/test.sh
index 7e1039f..6649d64 100755
--- a/t/test.sh
+++ b/t/test.sh
@@ -313,7 +313,8 @@ touch $D/a
WVPASS bup random 128k >$D/b
mkdir $D/d $D/d/e
WVPASS bup random 512 >$D/f
-WVPASS bup index -ux --exclude $D/d $D
+WVPASS bup random 512 >$D/g
+WVPASS bup index -ux --exclude $D/d --exclude $D/g $D
bup save -n exclude $D
WVPASSEQ "$(bup ls exclude/latest/$TOP/$D/)" "a
b
@@ -330,7 +331,8 @@ D=exclude-fromdir.tmp
EXCLUDE_FILE=exclude-from.tmp
echo "$D/d
$TOP/$D/g
-$D/h" > $EXCLUDE_FILE
+$D/h
+$D/i" > $EXCLUDE_FILE
rm -rf $D
mkdir $D
export BUP_DIR="$D/.bup"
@@ -340,6 +342,7 @@ WVPASS bup random 128k >$D/b
mkdir $D/d $D/d/e
WVPASS bup random 512 >$D/f
mkdir $D/g $D/h
+WVPASS bup random 128k >$D/i
WVPASS bup index -ux --exclude-from $EXCLUDE_FILE $D
bup save -n exclude-from $D
WVPASSEQ "$(bup ls exclude-from/latest/$TOP/$D/)" "a
--
1.7.9.2

[Daniel Hahler]

+1

I was surprised to see that files passed in an excludes-from-file made it into the backup and wondered if the excludes file is being recognized at all.

[Simon Persson]

Wouldn't it be simpler to just have one test if the path is excluded
at the top level in the for-loop, first thing?

Simon

[Gabriel Filion]

On 03/01/2013 01:41 AM, Simon Persson wrote:
>>> diff --git a/lib/bup/drecurse.py b/lib/bup/drecurse.py
>>> >> index 866045f..f72be25 100644
>>> >> --- a/lib/bup/drecurse.py
>>> >> +++ b/lib/bup/drecurse.py
>>> >> @@ -73,6 +73,12 @@ def _recursive_dirlist(prepend, xdev, bup_dir=None,
>>> >> excluded_paths=None):
>>> >>
>>> >> excluded_paths=excluded_paths):
>>> >> yield i
>>> >> os.chdir('..')
>>> >> + else:
>>> >> + if excluded_paths:
>>> >> + if os.path.normpath(prepend+name) in excluded_paths:
>>> >> + debug1('Skipping %r: excluded.\n' % (prepend+name))
>>> >> + continue
>>> >> +
>>> >> yield (prepend + name, pst)
>>> >>
> Wouldn't it be simpler to just have one test if the path is excluded
> at the top level in the for-loop, first thing?

I agree, that would be better.

--
Gabriel Filion

[Jakob Matthes]

Signed-off-by: Jakob Matthes <jakob....@gmail.com>
---

lib/bup/drecurse.py | 8 ++++----
t/test.sh | 7 +++++--
2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/bup/drecurse.py b/lib/bup/drecurse.py
index 2f4f286..5d49770 100644
--- a/lib/bup/drecurse.py
+++ b/lib/bup/drecurse.py
@@ -51,6 +51,10 @@ def _dirlist():

def _recursive_dirlist(prepend, xdev, bup_dir=None, excluded_paths=None):

for (name,pst) in _dirlist():

+ if excluded_paths:
+ if os.path.normpath(prepend+name) in excluded_paths:
+ debug1('Skipping %r: excluded.\n' % (prepend+name))
+ continue

if name.endswith('/'):
if xdev != None and pst.st_dev != xdev:
debug1('Skipping %r: different filesystem.\n' % (prepend+name))
@@ -59,10 +63,6 @@ def _recursive_dirlist(prepend, xdev, bup_dir=None, excluded_paths=None):

if os.path.normpath(prepend+name) == bup_dir:
debug1('Skipping BUP_DIR.\n')
continue
- if excluded_paths:
- if os.path.normpath(prepend+name) in excluded_paths:

- debug1('Skipping %r: excluded.\n' % (prepend+name))
- continue

try:
OsFile(name).fchdir()
except OSError, e:

diff --git a/t/test.sh b/t/test.sh
index 4211658..f52cdb4 100755
--- a/t/test.sh
+++ b/t/test.sh
@@ -363,7 +363,8 @@ touch $D/a

WVPASS bup random 128k >$D/b
mkdir $D/d $D/d/e
WVPASS bup random 512 >$D/f
-WVPASS bup index -ux --exclude $D/d $D
+WVPASS bup random 512 >$D/g
+WVPASS bup index -ux --exclude $D/d --exclude $D/g $D
bup save -n exclude $D
WVPASSEQ "$(bup ls exclude/latest/$TOP/$D/)" "a
b

@@ -380,7 +381,8 @@ D=exclude-fromdir.tmp

EXCLUDE_FILE=exclude-from.tmp
echo "$D/d
$TOP/$D/g
-$D/h" > $EXCLUDE_FILE
+$D/h
+$D/i" > $EXCLUDE_FILE

force-delete $D

mkdir $D
export BUP_DIR="$D/.bup"

@@ -390,6 +392,7 @@ WVPASS bup random 128k >$D/b

mkdir $D/d $D/d/e
WVPASS bup random 512 >$D/f
mkdir $D/g $D/h
+WVPASS bup random 128k > $D/i
WVPASS bup index -ux --exclude-from $EXCLUDE_FILE $D
bup save -n exclude-from $D
WVPASSEQ "$(bup ls exclude-from/latest/$TOP/$D/)" "a
--

1.8.1.5

[Gabriel Filion]

This looks good.

I've also run tests and a manual simulation and it seems to be doing
what it advertises.

Rob: I've tested this behaviour on 0.25-rc1 and it's already present.
the --exclude feature is even already present in 0.24b (although I
couldn't get "make" to work on that version for some unexplained reason)

so since it's not a regression from 0.25-rc1, you might decide to
postpone merging this patch, but I'd suggest merging it as soon as the
release was done.


Reviewed-by: Gabriel Filion <lel...@gmail.com>

Gabriel Filion

[Rob Browning]

Gabriel Filion <lel...@gmail.com> writes:

> so since it's not a regression from 0.25-rc1, you might decide to
> postpone merging this patch, but I'd suggest merging it as soon as the
> release was done.

> Reviewed-by: Gabriel Filion <lel...@gmail.com>

Pushed to master:

bup drecurse/index: allow non-directory --exclude and --exclude-from paths.

Previously bup only allowed directory exclusions.

Signed-off-by: Jakob Matthes <jakob....@gmail.com>
Reviewed-by: Gabriel Filion <lel...@gmail.com>
[r...@defaultvalue.org: change test file from g to j to avoid conflict
with subsequent "exclude-from" test; add WVPASS to mkdir so problem
is evident; edit commit message.]
Signed-off-by: Rob Browning <r...@defaultvalue.org>

I also pushed another patch that goes a little further with the
"exclude" and "exclude-from" tests, turning on "set -eo pipefail" for
both. It's a partial fix, but it should be an improvement.

When we move to more modular tests, I'll probably want "set -eo
pipefail" or an equivalent that fits in well with wvtest to be the
default.

Thanks
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4