[PATCH 6 of 6] vfs: also audit rename

[Yuya Nishihara]

On Fri, 21 Dec 2018 15:08:03 +0100, Boris FELD wrote:
> On 03/12/2018 12:03, Yuya Nishihara wrote:
> > 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.
> 
> So what should we do here?
> 
> 1) keep series as is
> 2) move rename in abstractvfs (keeping actual audit in `vfs`)
> 3) try to move __call__ in abstractbvfs (keep actual audit in `vfs`)
> 4) something else.

As I said before, my preference is (2).