[PATCH 6 of 6] vfs: also audit rename

[Yuya Nishihara]

On Sun, 2 Dec 2018 16:30:28 +0100, Boris FELD wrote:
> 
> On 28/11/2018 13:04, Yuya Nishihara wrote:
> > On Mon, 26 Nov 2018 19:22:48 +0100, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld at octobus.net>
> >> # Date 1498961267 -7200
> >> #      Sun Jul 02 04:07:47 2017 +0200
> >> # Node ID afdc73b20bd1faeec1b278ef0a2ab8d1dda71eb8
> >> # Parent  cc7d970d99eb242dbe2d8e792a9212aabd06911f
> >> # EXP-Topic vfs.audit-rename
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r afdc73b20bd1
> >> vfs: also audit rename
> > Queued the first 4 patches, thanks.
> >
> >> diff --git a/mercurial/vfs.py b/mercurial/vfs.py
> >> --- a/mercurial/vfs.py
> >> +++ b/mercurial/vfs.py
> >> @@ -436,6 +436,10 @@ class vfs(abstractvfs):
> >>  
> >>          return fp
> >>  
> >> +    def rename(self, src, dst, *args, **kwargs):
> >> +        self._auditpath(dst, 'w')
> >> +        return super(vfs, self).rename(src, dst, *args, **kwargs)
> > Can you move this to abstractvfs.rename()?
> >
> > I don't think it's worth to override rename() just for _auditpath(). Instead,
> > abstractvfs can implement _auditpath() that raises exception.
> 
> I'm not sure what are your concerns and what would be the point of
> moving that one layer up. Currently, the _auditpath logic is exclusive
> to vfs. It comes from the `vfs.__call__` method that the `abstractvfs`
> class do not have. What makes it worthy to move it in `abstractvfs`?
> 
> (I'm not opposed to doing so, I'm trying to understand the logic and
> responsibility of each class)

Since the abstractvfs provides most of the filesystem functions (except
__call__()), I just thought it would make sense to keep rename() in it.
I have no idea why __call__ isn't implemented by abstractvfs.

> > Also, 'dst' is likely to be relative in abstractvfs.rename(), so we won't
> > probably need to convert absolute 'dst' back to relative path.
> What's your reasoning here? We directly call the underlying `rename`
> method in the `vfs` class, and we know for sure that they can receive
> absolute paths.

Sorry, I misread the inheritance.

FWIW, I think the vfs is designed to receive relative paths, though absolute
path also works. It's probably wrong to access files out of the vfs.base.