[PATCH] umask for multiple commiters

[Aurelien Jacobs]

On Thu, 9 Nov 2006 17:47:40 +0100
"Benoit Boissinot" <bboissin at gmail.com> wrote:

> On 11/8/06, Aurelien Jacobs <aurel at gnuage.org> wrote:
> > On Thu, 2 Nov 2006 00:47:10 +0100
> > Aurelien Jacobs <aurel at gnuage.org> wrote:
> > > On Thu, 26 Oct 2006 11:08:04 +0200
> > > Thomas Arendsen Hein <thomas at intevation.de> wrote:
> > > > Maybe there are still too many stat/chmod calls, I suggest the
> > > > following:
> > > >
> > > > On repo object creation, look at the mode of the .hg and check if
> > > > the user's umask is permissive enough to create it.
> > > > Something like this code:
> > > >
> > > >     umask = os.umask(0)
> > > >     os.umask(umask)
> > > >     repomask = umask & ~st.st_mode
> > > >     if repomask != umask:
> > > >         self.ui.warn("Overriding your umask %04o with $04o for repository\n"
> > > >                 % (umask, repomask))
> > > >         self.repomask = repomask
> > > >     else:
> > > >         self.repomask = None
> > > >
> > > > And whenever writing to the repository (i.e. below .hg):
> > > >
> > > >     if self.repomask is not None:
> > > >         umask = os.umask(self.repomask)
> > > >     else:
> > > >         umask = None
> > > >     foo(bar, whatever)
> > > >     if umask is not None:
> > > >         os.umask(umask)
> > > >
> > > > Instead of changing with the umask, I can imagine doing a chmod on
> > > > file or directory creation, too, but only on creation, not every
> > > > time writing to a file.
> > >
> > > First let me say that this previous patch was already doing a chmod
> > > only for file/directory creation, not for simple file write.
> > >
> > > But I agree with you that a stat for each file creation might be a bit
> > > too much. So I improved this.
> > > I still kept chmod instead of changing umask. I guess that doing a chmod
> > > on a just created (ie. buffered) file is not worst than doing 2 umask call.
> > >
> > > About the complexity for creating N files:
> > >   * previous patch:
> > >      - N stat()
> > >      - N chmod()
> > >   * new patch:
> > >      - 1 stat() if N > 0
> > >      - N chmod() only if needed (ie. if current umask is not enough)
> > >
> > > I hope this patch is now ok.
> >
> > Did anyone had a look at it ?
> >
> I implemented Thomas' suggestion which I find cleaner (umask is a lot
> cheaper than chmod or stat, altough I don't know if it matters)

Are you sure that 2 umask syscall is cheaper that 1 chmod syscall on a
file that was just created (ie. inode is still in write buffer) ?

Anyway, I'm fine with your patch except for one detail. It does the couple
of umask call for every file write which is useless. umask is only needed
for file creation (and this makes a much bigger speed difference than
umask vs. chmod).
So the attached patch is a very small variation of yours, which do
umask only for file creation.
The patch is tested and working correctly.

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: umask2.diff
Type: text/x-diff
Size: 2412 bytes
Desc: not available
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20061112/e3fbe8a9/attachment.diff>