[PATCH 6 of 6] vfs: also audit rename

[Boris FELD]

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.
>
>>> 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.
There are various locations that use absolute paths (often within the
vfs itself). I don't think it is reasonable to update them and change
the API in the same series.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel