Patchwork [1,of,3,V3] vfs: allow to pass more argument to audit

# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1499768878 -7200
#      Tue Jul 11 12:27:58 2017 +0200
# Node ID 9addf65400ec8e6052a399c1586320988d73a321
# Parent  4672db164c986da4442bd864cd044512d975c3f2
# EXP-Topic vfs.ward
vfs: allow to pass more argument to audit

We want to be able to do more precise check when auditing a path depending of
the intend of the file access (eg read versus write). So we now pass the 'mode'
and 'atomictemp' value to 'audit' and update the audit function to accept them.

This will be put to use in the next changeset.

On Tue, 11 Jul 2017 17:55:27 +0200, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1499768878 -7200
> #      Tue Jul 11 12:27:58 2017 +0200
> # Node ID 9addf65400ec8e6052a399c1586320988d73a321
> # Parent  4672db164c986da4442bd864cd044512d975c3f2
> # EXP-Topic vfs.ward
> vfs: allow to pass more argument to audit

Looks generally good.

> We want to be able to do more precise check when auditing a path depending of
> the intend of the file access (eg read versus write). So we now pass the 'mode'
> and 'atomictemp' value to 'audit' and update the audit function to accept them.
> 
> This will be put to use in the next changeset.
> 
> diff -r 4672db164c98 -r 9addf65400ec mercurial/pathutil.py
> --- a/mercurial/pathutil.py	Sat Jun 24 15:29:42 2017 -0700
> +++ b/mercurial/pathutil.py	Tue Jul 11 12:27:58 2017 +0200
> @@ -46,7 +46,7 @@
>          else:
>              self.normcase = lambda x: x
>  
> -    def __call__(self, path):
> +    def __call__(self, path, mode=None, atomictemp=None):
>          '''Check the relative path.
>          path may contain a pattern (e.g. foodir/**.txt)'''
>  
> diff -r 4672db164c98 -r 9addf65400ec mercurial/vfs.py
> --- a/mercurial/vfs.py	Sat Jun 24 15:29:42 2017 -0700
> +++ b/mercurial/vfs.py	Tue Jul 11 12:27:58 2017 +0200
> @@ -306,7 +306,7 @@
>          if audit:
>              self.audit = pathutil.pathauditor(self.base)
>          else:
> -            self.audit = util.always
> +            self.audit =  (lambda path, mode=None, atomictemp=None: True)
>          self.createmode = None
>          self._trustnlink = None
>  
> @@ -360,7 +360,7 @@
>                  r = util.checkosfilename(path)
>                  if r:
>                      raise error.Abort("%s: %r" % (r, path))
> -            self.audit(path)
> +            self.audit(path, mode=mode, atomictemp=atomictemp)

Is 'atomictemp' needed? I don't think 'atomictemp' can be generalized well to
the other vfs operations. And atomictemp=True doesn't mean repo.lock/wlock
is unnecessary.

On Fri, 2017-07-14 at 00:28 +0900, Yuya Nishihara wrote:
> On Tue, 11 Jul 2017 17:55:27 +0200, Boris Feld wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld@octobus.net>
> > # Date 1499768878 -7200
> > #      Tue Jul 11 12:27:58 2017 +0200
> > # Node ID 9addf65400ec8e6052a399c1586320988d73a321
> > # Parent  4672db164c986da4442bd864cd044512d975c3f2
> > # EXP-Topic vfs.ward
> > vfs: allow to pass more argument to audit
> 
> Looks generally good.
> 
> > We want to be able to do more precise check when auditing a path
> > depending of
> > the intend of the file access (eg read versus write). So we now
> > pass the 'mode'
> > and 'atomictemp' value to 'audit' and update the audit function to
> > accept them.
> > 
> > This will be put to use in the next changeset.
> > 
> > diff -r 4672db164c98 -r 9addf65400ec mercurial/pathutil.py
> > --- a/mercurial/pathutil.py	Sat Jun 24 15:29:42 2017 -0700
> > +++ b/mercurial/pathutil.py	Tue Jul 11 12:27:58 2017 +0200
> > @@ -46,7 +46,7 @@
> >          else:
> >              self.normcase = lambda x: x
> >  
> > -    def __call__(self, path):
> > +    def __call__(self, path, mode=None, atomictemp=None):
> >          '''Check the relative path.
> >          path may contain a pattern (e.g. foodir/**.txt)'''
> >  
> > diff -r 4672db164c98 -r 9addf65400ec mercurial/vfs.py
> > --- a/mercurial/vfs.py	Sat Jun 24 15:29:42 2017 -0700
> > +++ b/mercurial/vfs.py	Tue Jul 11 12:27:58 2017 +0200
> > @@ -306,7 +306,7 @@
> >          if audit:
> >              self.audit = pathutil.pathauditor(self.base)
> >          else:
> > -            self.audit = util.always
> > +            self.audit =  (lambda path, mode=None,
> > atomictemp=None: True)
> >          self.createmode = None
> >          self._trustnlink = None
> >  
> > @@ -360,7 +360,7 @@
> >                  r = util.checkosfilename(path)
> >                  if r:
> >                      raise error.Abort("%s: %r" % (r, path))
> > -            self.audit(path)
> > +            self.audit(path, mode=mode, atomictemp=atomictemp)
> 
> Is 'atomictemp' needed? I don't think 'atomictemp' can be generalized
> well to
> the other vfs operations. And atomictemp=True doesn't mean
> repo.lock/wlock
> is unnecessary.

atomictemp wasn't technically necessary for this series, I included it
for completeness. If people starts wrapping pathauditor, adding
atomictemp later would results in breaking their wraps.

Do you want me to send a V4 to remove the atomictemp?

On Thu, 13 Jul 2017 22:11:11 +0200, Boris Feld wrote:
> > > +            self.audit(path, mode=mode, atomictemp=atomictemp)
> > 
> > Is 'atomictemp' needed? I don't think 'atomictemp' can be generalized
> > well to
> > the other vfs operations. And atomictemp=True doesn't mean
> > repo.lock/wlock
> > is unnecessary.
> 
> atomictemp wasn't technically necessary for this series, I included it
> for completeness. If people starts wrapping pathauditor, adding
> atomictemp later would results in breaking their wraps.

So we'll add e.g. 'src' of symlink() as well? IMHO, the interface of audit()
and ward() in the original series is pretty basic. 'mode' might make some
sense for all file operations, but 'atomictemp' wouldn't.

> Do you want me to send a V4 to remove the atomictemp?

Yeah, if that makes sense.