[PATCH] umask for multiple commiters

[Benoit Boissinot]

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)

please review,

Benoit
-------------- next part --------------
diff -r 7d3d603e7df9 mercurial/localrepo.py
--- a/mercurial/localrepo.py	Tue Nov 07 21:43:19 2006 +0100
+++ b/mercurial/localrepo.py	Thu Nov 09 17:44:57 2006 +0100
@@ -47,7 +47,16 @@ class localrepository(repo.repository):
         self.origroot = path
         self.ui = ui.ui(parentui=parentui)
         self.opener = util.opener(self.path)
-        self.sopener = util.opener(self.path)
+        # setup umask
+        umask = os.umask(0)
+        os.umask(umask)
+        repomask = ~stat.S_IMODE(os.stat(self.path).st_mode) & 0777
+        if repomask == umask:
+            repomask = None
+        else:
+            self.ui.warn(_("repository mode overrides umask with %04o\n"
+                           % repomask))
+        self.sopener = util.opener(self.path, umask=repomask)
         self.wopener = util.opener(self.root)
 
         try:
diff -r 7d3d603e7df9 mercurial/util.py
--- a/mercurial/util.py	Tue Nov 07 21:43:19 2006 +0100
+++ b/mercurial/util.py	Thu Nov 09 17:41:53 2006 +0100
@@ -754,7 +754,7 @@ else:
             return _("stopped by signal %d") % val, val
         raise ValueError(_("invalid exit code"))
 
-def opener(base, audit=True):
+def opener(base, audit=True, umask=None):
     """
     return a function that opens files relative to base
 
@@ -763,6 +763,7 @@ def opener(base, audit=True):
     """
     p = base
     audit_p = audit
+    umask_p = umask
 
     def mktempcopy(name):
         d, fn = os.path.split(name)
@@ -817,12 +818,15 @@ def opener(base, audit=True):
     def o(path, mode="r", text=False, atomic=False, atomictemp=False):
         if audit_p:
             audit_path(path)
+        old_umask = None
         f = os.path.join(p, path)
 
         if not text:
             mode += "b" # for that other OS
 
         if mode[0] != "r":
+            if umask_p is not None:
+                old_umask = os.umask(umask_p)
             try:
                 nlink = nlinks(f)
             except OSError:
@@ -836,7 +840,10 @@ def opener(base, audit=True):
                     return atomictempfile(f, mode)
                 if nlink > 1:
                     rename(mktempcopy(f), f)
-        return posixfile(f, mode)
+        fh = posixfile(f, mode)
+        if old_umask is not None:
+            os.umask(old_umask)
+        return fh
 
     return o