[PATCH] umask for multiple commiters

[Aurelien Jacobs]

On Tue, 24 Oct 2006 01:31:19 -0300
"Alexis S. L. Carvalho" <alexis at cecm.usp.br> wrote:

> Thus spake Aurelien Jacobs:
> > # HG changeset patch
> > # User Aurelien Jacobs <aurel at gnuage.org>
> > # Date 1161034334 -7200
> > # Node ID 29513230e3858b572c6e2fb1d60bcb15b11ab2a4
> > # Parent  5207cf649abe173239db83cabf68ea8dedddfd77
> > inherit parent directory mode when creating new file
> 
> I don't have a strong opinion on the feature (I think some users will
> like it, while others will say that one shouldn't reinvent the wheel and
> just use umask), but...

I don't care if it's implemented this way or with umask (like in my
initial patch), as long as one of them is picked and applied.

> > diff -r 5207cf649abe -r 29513230e385 mercurial/localrepo.py
> > --- a/mercurial/localrepo.py	Mon Oct 16 12:56:41 2006 +0200
> > +++ b/mercurial/localrepo.py	Mon Oct 16 23:32:14 2006 +0200
> > @@ -34,10 +34,7 @@ class localrepository(repo.repository):
> >  
> >          if not os.path.isdir(self.path):
> >              if create:
> > -                if not os.path.exists(path):
> > -                    os.mkdir(path)
> > -                os.mkdir(self.path)
> > -                os.mkdir(self.join("data"))
> > +                util.makedirs(self.join("data"))
> 
> I do think that repo creation should just follow the umask - I don't see
> why you would want to inherit the permissions from the parent in this
> specific case.

Example of a situation: someone want to create a new repo on the
server with "hg init ssh://server//dir/repo". If it's not created
with parents mode, it won't be group writable and thus other users
won't be able to commit on it.
Telling users to chmod after a hg init is not an option as their
shell is the restricted hgsh.

But I can also see how this feature could be controversial.

> (And this patch hunk would probably change the current "hg init doesn't
> create intermediate directories" behaviour that was just discussed on
> the other list).

Indeed. I personnaly prefer this new behavior, but I guess others don't.
Obviously such a behavior change shouldn't be tied to my patch.

Anyway, I removed that hunk.

> > diff -r 5207cf649abe -r 29513230e385 mercurial/util.py
> > --- a/mercurial/util.py	Mon Oct 16 12:56:41 2006 +0200
> > +++ b/mercurial/util.py	Mon Oct 16 23:32:14 2006 +0200
> > @@ -423,6 +423,20 @@ def system(cmd, environ={}, cwd=None, on
> >          if cwd is not None and oldcwd != cwd:
> >              os.chdir(oldcwd)
> >  
> > +def makedirs(name):
> 
> A docstring would be nice.

Right.

> > +    try:
> > +        os.mkdir(name)
> > +        os.chmod(name,os.lstat(os.path.abspath(os.path.dirname(name))).st_mode)
> 
> You really don't want to use lstat to get the permissions (if it's a
> symlink, you'll get the permissions of the symlink itself - 0777).

I don't even know why I used lstat ! Probably a copy/paste from somewhere.

> And this line is starting to get a bit too hard to read.

Ok.

> > @@ -802,7 +817,8 @@ def opener(base, audit=True):
> >              except OSError:
> >                  d = os.path.dirname(f)
> >                  if not os.path.isdir(d):
> > -                    os.makedirs(d)
> > +                    makedirs(d)
> > +                st_mode = os.lstat(d).st_mode & 0666
> 
> lstat again.
> 
> > @@ -810,7 +826,10 @@ def opener(base, audit=True):
> >                      return atomictempfile(f, mode)
> >                  if nlink > 1:
> >                      rename(mktempcopy(f), f)
> > -        return posixfile(f, mode)
> > +        pf = posixfile(f, mode)
> > +        if st_mode != None:
> 
> "st_mode is not None" is more idiomatic - but I'm already nitpicking.

Damn, I'm spotted ! Indeed, I'm not a python coder. I'm a C coder.
So don't hesitate to nitpick if my code is not exactly pythonish.

Updated patch attched.
It now only affect file/directory creation inside .hg.
Mode setting on .hg itself must still be done by hand.
So this should really be safe.

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inherit-mode.diff
Type: text/x-diff
Size: 2003 bytes
Desc: not available
Url : http://www.selenic.com/pipermail/mercurial-devel/attachments/20061025/dafe0f2e/inherit-mode.bin